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

add to CONTRIBUTING.rst how to update master compute data #922

Merged
merged 14 commits into from
Mar 6, 2024
Merged
38 changes: 38 additions & 0 deletions CONTRIBUTING.rst
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,44 @@ Opening a PR will trigger a suite of tests and style/formatting checks that must
We also require approval from at least one (ideally multiple) of the main DESC developers, who may have suggested changes
or edits to your PR.

What if the ``test_compute_everything`` test fails, or there is a conflict in ``master_compute_data.pkl``?
----------------------------------------------------------------------------------------------------------
When the outputs of the compute quantities tested by the`test_compute_everything` [test](https://github.com/PlasmaControl/DESC/blob/master/tests/test_compute_funs.py) are changed in a PR, that test will fail.
The three main reasons this could occur are:

- The PR was not intended to change how things are computed, but messed up something unexpected and now the compute quantities are incorrect, if you did not expect these changes in the PR then look into why these differences are happening and fix the PR.
- The PR updated the way one of the existing compute index quantities are computed (either by a redefinition or perhaps fixing an error present in ``master``)
- The PR added a new class parametrization (such as a new subclass of ``Curve`` like ``LinearCurve`` etc)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this actually make the test fail? I don't think it will, but you still have to remember to add that new parameterization to the test as you explained. Also this applies to adding entirely new classes, not just new parameterizations of an existing class

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also "parameterization" is missing a second "e"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and yea it does fail bc of this line in test_compute_everything

assert things.keys() == data_index.keys(), (
        f"Missing the parameterizations {data_index.keys() - things.keys()}"
        f" to test against master."
    )


If the 2nd case is the reason, then you must update the ``master_compute_data.pkl`` file with the correct quantities being computed by your PR:

- First , run the test with ``pytest tests -k test_compute_everything`` and inspect the compute quantities whose values are in error, to ensure that only the quantities you expect to be different are shown (and that the new values are indeed the correct ones, you should have a test elsewhere for that though).
dpanici marked this conversation as resolved.
Show resolved Hide resolved
- If the values are as expected and only the expected compute quantities are different, then change the block

```python
except AssertionError as e:
error = True
print(e)
```
with

```python
except AssertionError as e:
error = False
update_master_data = True
print(e)
```

- rerun the test ``pytest tests -k test_compute_everything`` , now any compute quantity that is different between the PR and master will be updated with the PR value
dpanici marked this conversation as resolved.
Show resolved Hide resolved
- ``git restore tests/test_compute_funs.py`` to remove the change you made to the test
- ``git add tests/inputs/master_compute_data.pkl`` and commit to commit the new data file

If the 3rd case is the reason, then you must simply add the new parametrization to the ``test_compute_everything`` [test](https://github.com/PlasmaControl/DESC/blob/master/tests/test_compute_funs.py)

- ``things`` dictionary with a sensible example instance of the class to use for the test, and
- to the ``grid`` dictionary with a sensible default grid to use when computing the compute quantities for the new class
- Then, rerunning the test ``pytest tests -k test_compute_everything`` will add the compute quantities for the new class and save them to the ``.pkl`` file
- ``git add tests/inputs/master_compute_data.pkl`` and commit to commit the new data file

Styleguides
^^^^^^^^^^^
Expand Down
Loading