Skip to content

Conversation

@jodemaey
Copy link
Member

No description provided.

@jodemaey jodemaey requested a review from ushham March 26, 2025 13:52
@jodemaey
Copy link
Member Author

Hi @ushham ,

I checked the Fortran tabulation issue and it looks fine.

On the other hand, setting continuation_variables=None is currently similar to continuation_variables=[] and returns tendencies where all the substitutions have been done. So there is a problem there.

I think that after this is solved we can merge.

Cheers,

Jonathan

@ushham
Copy link
Collaborator

ushham commented Mar 27, 2025

I have fixed the above issue that you raised. It was occurring because there was a mix of using parameter objets and a dict storing the symbols. The use of the dict was a hangover from earlier versions of this and should have been removed once you implemented the new parameter class.

@jodemaey
Copy link
Member Author

Regarding this comment, actually when I generate fully symbolic tendencies, there is no trace of pi, neither in Python nor in Fortran. I think Sympy already replaces it in the inner products, so I think we can drop pi from these dicts.

image

@ushham
Copy link
Collaborator

ushham commented Mar 28, 2025

I wonder if the outputs of the inner products still include pi, I don't think I would have included this for no reason, but its clearly not needed so can be removed

@jodemaey
Copy link
Member Author

If think it depends on the way you ask Sympy to output them, but clearly here the pi's are already converted to numerics.

@ushham
Copy link
Collaborator

ushham commented Mar 28, 2025

Regarding this comment, actually when I generate fully symbolic tendencies, there is no trace of pi, neither in Python nor in Fortran. I think Sympy already replaces it in the inner products, so I think we can drop pi from these dicts.

image

I also note that I was subbing conjugations which are not needed. I can remove these

@jodemaey
Copy link
Member Author

When you increase resolution, at some point conjugations appear.

@jodemaey
Copy link
Member Author

jodemaey commented Mar 28, 2025

Good, in the end, the user can still substitute expressions if he wants anyway.
I think we are good to merge. @ushham , can you formally make the review and accept the PR, then I can merge.

Copy link
Collaborator

@ushham ushham left a comment

Choose a reason for hiding this comment

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

Yes exactly, it is still flexible for subs from the user. Ok all looks good to me :)

@jodemaey jodemaey merged commit cebf6e2 into develop Mar 28, 2025
11 checks passed
@jodemaey
Copy link
Member Author

Ok this is merged. I will proceed to merge to master and then release. Good job !

@jodemaey jodemaey deleted the doc/symbolic_output branch March 28, 2025 13:06
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.

3 participants