Conversation
|
Hmm so we are getting into dangerous territory here with the code version becoming important. If we just implement the changes here as is then we break backwards compatibility with already existing CGYRO runs. CGYRO does store its git commit here so we could do some fancy stuff here with checking git commits and checking when that occurred though that could be a bit onerous. I think removing the @ZedThree thoughts on how to proceed? |
|
I agree that it's a bit annoying about the code versions. To aid decision-making, the |
|
Broadly speaking, I think there are two ways of dealing with things like this: either have separate implementations for incompatible versions of a code (but that use a common base class), or have conditionals in the one implementation. Separate implementations for different versions might add a bunch of complexity, but the benefit is that they can be named, which would allow users to explicitly do cross-version comparisons -- In any case, it might be difficult to deal with builds from between releases -- how do you tell if a run was down before or after a particular bug fix or change in behaviour if it's not exactly version |
|
I don't know if this is bad etiquette to add this change to the pre-exisitng PR but I thought it was less work that create a new branch and PR. |
… from external netcdf.
…cs/pyrokinetics into bugfix/cgyro_various
|
I think I'm happy enough for now so will merge and see what problems arise! |
src/pyrokinetics/gk_code/cgyro.py
Outdated
|
|
||
| with open(filename, "w") as f: | ||
| for key, value in self.data.items(): | ||
| if key in ["Z_EFF", "Z_EFF_METHOD"]: |
There was a problem hiding this comment.
We should actually do something before the open like
del self.data["Z_EFF"]
This PR is intended to update PYRO to be compatible with some of the more recent CGYRO changes. Specifically, these are:
A CGYRO recent commit changed the sign of the normalisation of the QL fluxes. If I am tracking the definitions of correctly in cgyro_make_profiles.F90, this means that the sign of
flux_normshould be modified as shown. This gives good agreement of the QL computed before and after the change, for a case I have, but this breaks the automatic tests.CGYRO is doing away with
Z_EFFandZ_EFF_METHODas input parameters (these will give theCGYRO: bogus parametermessage). The easiest way to handle these in PYRO is to remove them fromset(), meaning that the user will have to specify them manually now if they want behaviour different from the current functionality ofZ_EFF_METHOD=2.Happy to take feedback @bpatel2107