Updates to the _parse_parameter_file function following PR 4470

[mtryan83]

PR Summary

This change updates the _parse_parameter_file function in the Gadget frontend to match the changes developed in PR #4470. In summary:

1. Change try/except to if/else
2. Remove only_on_root wrapper to mylog.info
3. Change fall-back omega_matter from 1 to 0
4. Condense prefix logic

I've left the for/else structure in, since in #4470 I just removed it (didn't need to support Gadget-2 format for Gizmo files) and so haven't fully thought of what the replacement should look like.

PR Checklist

• New features are documented, with docstrings and narrative docs
• Adds a test for any bugs fixed. Adds tests for new features.

I believe this neither adds new features, nor fixes any bugs, and is more of a code cleanup than anything else.

[neutrinoceros]

just a small comment. I'll review properly tomorrow,

[neutrinoceros]

actually I don't have other comments, LGTM

[jzuhone]

I guess my question is if this is now in line with PR #4470, then it seems like we should have done this first and then tried to find a way for the GIZMO frontend's method to call this one as super and then do the things it needed to do. But maybe that's not possible.

[chummels]

So it seems like PR #4470 went through because of changes in the Gizmo frontend, but do we know that equivalent changes occurred in the underlying Gadget code as well? My concern is that this might break how we're loading in Gadget cosmological simulations, since we're no longer assuming Redshift to be set in the underlying dataset, and now basing our assessment of cosmology on the presence of the OmegaLambda parameter.

[mtryan83]

I don't know of any changes to the Gadget frontend, but my understanding is that nothing here should change the logic of how the parameter file is parsed. For example, the Redshift parameter was assumed but not guaranteed and either way did not effect whether yt treated it as a cosmological dataset:

# previous version of the code
try:
    self.current_redshift = hvals["Redshift"]
except KeyError:
    # Probably not a cosmological dataset, we should just set
    # z = 0 and let the user know
    self.current_redshift = 0.0
    only_on_root(mylog.info, "Redshift is not set in Header. Assuming z=0.")

and then

# Previous version (with info statement removed for clarity)
if self.omega_lambda == 0.0:
    ### ... Not cosmological ...
    self.cosmological_simulation = 0
    self.current_redshift = 0.0 ### This is set to 0, even if it wasn't before

Please feel free to correct me though and, of course, I have no idea if the previous logic is correct.

To @jzuhone 's point, I believe you are correct. In hindsight, I think the Gizmo frontend could call super and then just reevaluate only the omega/cosmology section (i.e. lines 370-409). I don't have time to work on this before mid-September though.

[JBorrow]

Why change this to 0.0, instead if 1.0? This would imply there's nothing in the volume.

[mtryan83]

From @neutrinoceros

non-cosmological datasets normally have omega_matter set to 0.0. Is there a reason not to follow the convention here ?

in PR #4470

[JBorrow]

Ok, so it was changed for consistency with other frontends? That's ok, but maybe warrants changing the comment from 'Just incase somebody asks for it' to 'Changed to 0.0 for consistency with non-cosmological frontends' in case somebody comes looking?

[mtryan83]

Fair point. The comment was actually left unchanged from the previous code (and I believe is just referencing that omega_matter is being set, not necessarily it's value), I had only changed the numerical value here.

I've added your comment, though I left the original as well, since it does describe why omega_matter is being set at all.