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

Update perturbIC.py #5

Open
wants to merge 190 commits into
base: main
Choose a base branch
from

Conversation

leoberhelman
Copy link
Contributor

This is a draft of a pull request to keep track of comments and questions I will have during refactoring and testing. Nothing yet but I am creating my unit tests.

@leoberhelman leoberhelman self-assigned this Oct 16, 2024
@leoberhelman leoberhelman linked an issue Oct 16, 2024 that may be closed by this pull request
Can the random generator be simplified?
src/perturbIC.py Outdated Show resolved Hide resolved
…er functions. The descriptions are still in progress.
@MartinDix
Copy link
Contributor

I can't work out why I specified it this way. In any case RandomState is no longer the recommended generator and the more up to date way of doing it would be

from numpy.random import Generator, PCG64
rg = Generator(PCG64(seed))
.. .rg.random(...)

This will give different results to the current implementation but will be stable in the future. PCG64 is currently the default bit generator but that could change so it's used explicitly here.

leoberhelman and others added 4 commits November 21, 2024 10:51
Co-authored-by: Davide Marchegiani <[email protected]>
Co-authored-by: Davide Marchegiani <[email protected]>
Co-authored-by: Davide Marchegiani <[email protected]>
Co-authored-by: Davide Marchegiani <[email protected]>
Copy link
Contributor Author

@leoberhelman leoberhelman left a comment

Choose a reason for hiding this comment

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

I made the changes and a few more to correct some errors. Still need to update the unit tests

Copy link
Collaborator

@atteggiani atteggiani left a comment

Choose a reason for hiding this comment

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

Thank you @leoberhelman for the amazing work restructuring this after my extensive review!

I added a few other minor suggestions. Please refer to the specific comments.

Two important points:

  • I am going to open up a PR with a revision of the testing suite. That can also serve as a guideline an example for future testing suites.
    As a consequence, there is no need to address the specific comment I left regarding the test_create_default_outname function. I will include that part in my testing-suite PR. The test_create_default_outname comment should be viewed as an example of a testing approach.
  • With the restructuring of the repo and creation of the original branch, I think the main branch should now only include the portions of the codebase we are restructuring, and potentially be in a "releasable" state (in the not-too-far future). Therefore, I would delete all the scripts that we are not using from this branch.
    Namely, I would only keep the perturbIC.py script (moving it inside the umfile_utils folder, along with the already present _version.py and __init__.py files) while movind the README.md at the top-level of the repo.
    Note that, after this restructure, some minor modifications might be needed with the imports (from perturbIC import ... should become from umfile_utils.perturbIC for example). Also, in the pyproject.toml file the following lines should be added to ensure pytest works correctly:
    [tool.pytest.ini_options]
    pythonpath = "src"

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
environment-dev.yml Outdated Show resolved Hide resolved
src/perturbIC.py Outdated Show resolved Hide resolved
src/perturbIC.py Show resolved Hide resolved
Don't perform the validation, but print a message to inform that validation has been skipped.
"""
print('Skipping mule validation. To enable the validation, run using the "--validate" option.')
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return

We don't really need to return anything here I believe.

src/perturbIC.py Outdated Show resolved Hide resolved
src/perturbIC.py Outdated Show resolved Hide resolved
Comment on lines +201 to +202
# Skip mule validation if the "--validate" option is provided
if args.validate:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Skip mule validation if the "--validate" option is provided
if args.validate:
# Perform mule validation only if the "--validate" option is provided
if not args.validate:

tests/perturbIC_test.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@atteggiani atteggiani left a comment

Choose a reason for hiding this comment

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

Just added another comment

src/perturbIC.py Outdated Show resolved Hide resolved
leoberhelman and others added 12 commits December 2, 2024 15:04
Co-authored-by: Davide Marchegiani <[email protected]>
Co-authored-by: Davide Marchegiani <[email protected]>
Co-authored-by: Davide Marchegiani <[email protected]>
Co-authored-by: Davide Marchegiani <[email protected]>
Co-authored-by: Davide Marchegiani <[email protected]>
Co-authored-by: Davide Marchegiani <[email protected]>
Co-authored-by: Davide Marchegiani <[email protected]>
Co-authored-by: Davide Marchegiani <[email protected]>
Co-authored-by: Davide Marchegiani <[email protected]>
Co-authored-by: Davide Marchegiani <[email protected]>
Update to unit tests
fix the args set up
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.

Requirements for perturbIC.py
3 participants