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

remove global_params #452

Merged
merged 24 commits into from
Feb 13, 2025
Merged

remove global_params #452

merged 24 commits into from
Feb 13, 2025

Conversation

daviesje
Copy link
Contributor

I'm setting up a draft PR for removing the global parameter structure, as many of these are under/unused.

For now I've removed the parameters which were completely unused and I've commented on the others with suggestions of what to do with them.

I will begin to go through the parameters and move/delete them as necessary.

Copy link

codecov bot commented Jan 22, 2025

Codecov Report

Attention: Patch coverage is 89.54248% with 16 lines in your changes missing coverage. Please review.

Project coverage is 79.25%. Comparing base (5930245) to head (8879153).
Report is 30 commits behind head on v4-prep.

Files with missing lines Patch % Lines
src/py21cmfast/wrapper/outputs.py 14.28% 2 Missing and 4 partials ⚠️
src/py21cmfast/wrapper/structs.py 72.22% 3 Missing and 2 partials ⚠️
src/py21cmfast/wrapper/inputs.py 94.87% 0 Missing and 2 partials ⚠️
src/py21cmfast/wrapper/photoncons.py 93.10% 1 Missing and 1 partial ⚠️
src/py21cmfast/_cfg.py 97.14% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           v4-prep     #452      +/-   ##
===========================================
- Coverage    79.56%   79.25%   -0.32%     
===========================================
  Files           24       23       -1     
  Lines         3803     3735      -68     
  Branches       647      638       -9     
===========================================
- Hits          3026     2960      -66     
- Misses         558      560       +2     
+ Partials       219      215       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@daviesje
Copy link
Contributor Author

daviesje commented Feb 11, 2025

A few notes here:

  • I have removed (mostly commented) the WDM model that existed via global_params.P_CUTOFF since it seemed inconsistent.
  • I have forced the global_params.USE_ADIABATIC_FLUCTUATIONS to True. This seemed like something we always wanted to be true.
  • I am thinking about removing the DEXM_OPTIMIZE model, but this should probably go in another PR

Paths and memory allocation factors were moved into the config, and the config file (along with its warnings) are gone, substituted by a single instance which automatically passes necessary information to the backend via a new very small struct ConfigSettings.

Copy link
Member

@steven-murray steven-murray left a comment

Choose a reason for hiding this comment

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

@daviesje thanks, this is super nice. I think I agree with everything you've done here. I would say that the config class has actually become almost unnecessary now, so maybe we should htink about whether it is still reuqired (or whether it should only be kept around for these lingering globals).

Also, I'm not sure I totally agree with all the places you're putting parameters. But I could be convinced. In my head, AstroParams are for actual physical parameters that can be constrained, while FlagOptions are for discrete options (True/False or choices from a list). This leaves no actual place for options that are continuous, but not a physical parameter (e.g. MAX_DVDR). So I'm not sure what the best place for those are. We should perhaps chat about this.

Comment on lines +27 to 32
"direc": Path("~/21cmFAST-cache").expanduser(),
"regenerate": False,
"write": True,
"cache_param_sigfigs": 6,
"cache_redshift_sigfigs": 4,
"ignore_R_BUBBLE_MAX_error": False,
Copy link
Member

Choose a reason for hiding this comment

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

I actually don't think any of these settings are being used any more :/ We probably should be using the sigfigs -- we shouldn't lose that functionality. But I don't think the new output struct framework actually accesses this info any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove these as necessary, but I think it's a good idea to have some place we can put variables which don't affect the run output, and may need to be passed in.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think having this structure around makes sense. I think for this PR, let's leave everything in there. Once we merge the output struct PR, we'll have a better sense of what is still required.

src/py21cmfast/_cfg.py Outdated Show resolved Hide resolved
@daviesje
Copy link
Contributor Author

As for the placement of the globals, I only considered whether they are utilised before or after the PerturbField finishes. Since we tie the parameter sets to these structs I thought it was more important to use that distinction, even if categorically they make less sense. We can talk about alternate ways to organise the parameters.

@steven-murray
Copy link
Member

Yeah, the before/after PerturbedField is obviously a very important distinction.

@daviesje daviesje marked this pull request as ready for review February 13, 2025 09:55
@daviesje daviesje mentioned this pull request Feb 13, 2025
@daviesje
Copy link
Contributor Author

daviesje commented Feb 13, 2025

More notes:

  • I've updated the upload-artifact action since version three was deprecated by Github
  • I've folded in some fixes that were in a separate PR Fixing photoncons #454 since they were needed to fix some tests
  • A bunch of issues were found and fixed by removing the **global_kwargs arguments from all the functions, since previously removed arguments to functions were being lumped in there and errors weren't being thrown if these were not global parameters
  • There are some TODO notes where I believe my implementation is messy, and I would appreciate any feedback / ideas on how to improve them

)
) + ")"


# TODO: force config to be a singleton
Copy link
Member

Choose a reason for hiding this comment

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

An easy way to achieve some of this is just to make Config semi-private... i.e. _Config. It actually doesn't matter if someone creates a new instance of _Config -- it'll just never be used.

Copy link
Member

@steven-murray steven-murray left a comment

Choose a reason for hiding this comment

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

Thanks @daviesje. I made a couple of comments, but none of them are absolutely necessary. I am a little scared of how this is going to interact with #445. Might be a nasty merge.

Comment on lines +369 to +372
validator=[
validators.in_(_filter_options),
validators.not_(validators.in_("sharp-k")),
], # TODO: seems bad
Copy link
Member

Choose a reason for hiding this comment

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

alternatively,

validator = validators.in_([x for x in _filter_options if x!='sharp-k'])

@@ -36,14 +36,5 @@ def test_config_write(cfgdir):
with open(cfgdir / "config.yml", "w") as fl:
yaml.dump(new_config, fl)

with pytest.warns(UserWarning):
with pytest.raises(ConfigurationError):
Copy link
Member

Choose a reason for hiding this comment

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

I think we should just get rid of all config abilities to load/write files.

@daviesje
Copy link
Contributor Author

Note to move some of the AstroParams which we won't want to infer into FlagOptions for a future renaming

@daviesje daviesje merged commit 2ee4c2a into v4-prep Feb 13, 2025
7 of 14 checks passed
@daviesje daviesje deleted the delete_globals branch February 13, 2025 13:45
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.

None yet

2 participants