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

Implement Cartesian option as a config key (instead of the current compiler flag) #33

Open
RobinSattler opened this issue Dec 31, 2024 · 7 comments

Comments

@RobinSattler
Copy link
Contributor

I used the branch cartesian_option for some time now and it would be nice to refactor the Cartesian option from a compiler flag into a config key and eventually merge it back into stable_ebe.

  • @yukarpenko, are there any reasons against this idea? If not, I would start working on it.
  • @NGoetz, does it make sense to keep (and fix) the Cartesian VTK output transformation (accessible via the config key VTK_Cartesian) which is implemented right now in src/vtk.cpp in this case or should we get rid of it entirely?
@NGoetz
Copy link
Contributor

NGoetz commented Dec 31, 2024

I think the transformation still makes sense in cases where the run is done in Milne but the vtk should be in cartesian

@RobinSattler
Copy link
Contributor Author

Alright, I thought so too, then I'll fix the Cartesian VTK output first. Thanks!

@yukarpenko
Copy link
Owner

yukarpenko commented Jan 1, 2025

It is definitely a good idea! Are you thinking about it in the context of dynamical fluidisation?
In any case, it should be done carefully, and it may take some efforts to do so.

  • first, I am actually not 100% sure about the correctness of the Cartesian version - I did not do any big testing. So it would be good to test whether it even works OK.
  • if it does, merging should better be done carefully, since we are amending the core algorithm. There is a bit elevated risk to break things in this case.

If you have an idea how to do so, maybe we discuss it first. I certainly appreciate your readiness to do it (remember the manpower argument I mentioned in a different discussion)! With the existing and planned changes it might be actually worthwhile to make a publication about the 2025 code as compared to the 2014 reference I did back then.

@RobinSattler
Copy link
Contributor Author

Okay, perfect. Thanks for your input! The dynamical fluidisation will benefit from this as well and I will discuss with Renan and Zuzana, although it's not my focus. I would only do the Cartesian config key implementation and leave further changes regarding the dynamical fluidisation to somebody else (if that is desired to have in stable_ebe as well).

I'll come back to this issue and start a discussion about how to tackle the testing and the implementation when I fixed the Cartesian VTK output.

@RobinSattler
Copy link
Contributor Author

After handling the VTK source code fix, I started looking into the Cartesian option.

  • I considered only the cartesian_option branch of Zuzana up to now because she already did a few tests in the beginning of 2023 checking her branch (documented in smash-devel issue #954; I don't know if you have access, @yukarpenko?). In the issue is mentioned that it was coordinated with you, @yukarpenko. The tests Zuzana did were (quoted from the issue; if needed, I can post the her plots into this issue):

    1. Shocktube test: energy density and velocity, I checked it against Yuriy's original results and it looks very similar
    2. Symmetric radial expansion: this is initialized with a higher energy density at [0,0,0] within a sphere (here 8 GeV/fm3), lower e around (1 GeV/fm3 here). The evolution should preserve the symmetry. I plot here the region where energy density is in [4,5] GeV/fm3 range, it is clearly spherical.

    In the issue was concluded:

    The code works as expected in test cases [...].

    I don't know if this is sufficient to consider the Cartesian option to be working in a correct way. If so, this would be a possible testing framework to be used after the implementation of the config key.

I went on to check a few things.

  • I started with a comparison between the cartesian_option branch of Iurii (commit 1c3708b; important: I removed the -D CARTESIAN flag in Makefile for this comparison) and the commit before that one (27eda0b; so right before the Cartesian option implementation). So both runs were in Milne. I just wanted to check if the Cartesian implementation messed up some code behavior in Milne. The two freezeout.dat files were identical, so this should be fine for now.
  • I did the same comparison for the other two commits in Zuzana's cartesian_option branch. Only the last commit in there (commit 39cba1d) introduced something that led to values differ ever so slightly in the freezeout.dat files.

For my checks I used the following config and SMASH IC file.
config.txt
SMASH_IC.txt

Maybe @yukarpenko and @zpauliny could look into this? Is it obvious to one of you why the value changes happen? And is this implementation (or more precisely the added lines) by Zuzana in commit 39cba1d something that should only be considered in the Cartesian runs or is this an improvement for Milne as well?

@yukarpenko
Copy link
Owner

yukarpenko commented Jan 16, 2025

I looked the commits up and yes, 39cba1d in Zuzana's cartesian_option branch does introduce what looks a more symmetric treatment of different directions in the code (in the basic tau-eta evolution, the eta direction is always treated in a bit different way from X and Y; in Cartesian frame it shouldn't happen), so it looks ok to me.
[update: forgot to add] yes, as a result of the commit 39cba1d, the evolution is also slightly different in the tau-eta frame. So the behaviour you're seeing looks ok to me, doesn't look like a bug.

@RobinSattler
Copy link
Contributor Author

@yukarpenko, thanks for the input! Then I will continue using Zuzana's branch for the implementation.

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

No branches or pull requests

3 participants