Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TRACE Main PR #771

Closed
wants to merge 100 commits into from
Closed

Conversation

tigerchenlu98
Copy link
Contributor

Hi Hanno,

Here's the hopefully final PR! Things look pretty clean (except the notebooks, don't think there's a way around that). I replaced all the examples, the only one which is doing worse is PrimordialEarth - not sure why, I need to look into it more.

Let me know if everything looks good!

Tiger

This was referenced May 5, 2024
@hannorein
Copy link
Owner

Wow! This looks great! I looked over it just now and only found very minor things. I want to stare at it a little longer, just to make sure. But I think we're basically done!


- Default switching function

This is a similar (but slightly modified) switching function used in MERCURY and MERCURIUS.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be a bit expanded. What is the default switching function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know how much detail we want to go into in the docs, but how does it look now?

docs/integrators.md Outdated Show resolved Hide resolved
@hannorein
Copy link
Owner

Please merge in my branch tigerchenlu98-newTRACE4_FULL_fbs for some minor changes/cleanup.

Then see the two small queries above.

@tigerchenlu98
Copy link
Contributor Author

Both done!

@hannorein
Copy link
Owner

I pushed a few updates to the notebooks (you can merge my tigerchenlu98-newTRACE4_FULL_fbs branch again). Mainly typos, consistency things.

Not sure why the windows tests sometimes (?) fail. Does not seem related to TRACE.

We either need to revert the primordial earth example to use mercurius, or update it (it still mentions Mercurius), depending on what your tests show.

@hannorein
Copy link
Owner

Squash merged. I kept the old Primordial Earth for now. We can update it any time if we want to.

@hannorein hannorein closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants