run_checks_compute() bugfix #837
Conversation
The run_checks_compute() function included the internal "_default" compute parameter set in checking the ck2004 model atmosphere table existence in the passband files. This caused the checks to fail even if ck2004 was never needed.
|
Note that this fix had to be applied to the blending-dev branch so that other commits can be tested/applied. |
phoebe/frontend/bundle.py
Outdated
| @@ -1,6 +1,7 @@ | |||
| import sys | |||
| import subprocess | |||
| import os | |||
| import numpy as np | |||
There was a problem hiding this comment.
this seems to be unused in this PR
There was a problem hiding this comment.
right, but np is used throughout bundle and we rely on a circular import, which isn't good practice.
There was a problem hiding this comment.
we get it from importing all from parameters, right? (Not really a circular import... but importing * surely isn't the best, let's make a ticket for that to refactor and explicitly import things, but that definitely shouldn't happen here)
|
We will also need to make sure to update any changes made to #826 so changes made during review here are not later reverted. |
It's implicitly tested in the blackbody extinction test in PR #826, so we do have coverage. |
phoebe/frontend/bundle.py
Outdated
| # NOTE: atmparam includes a '_default' compute pset, which depends on the | ||
| # ck2004 atmospheres; the checks should not include it. This is achieved | ||
| # by filtering check_visible=True in the line below: | ||
| for atmparam in self.filter(qualifier='atm', kind='phoebe', compute=computes, check_visible=True, check_default=False).to_list() + self.filter(qualifier='ld_coeffs_source').to_list(): |
There was a problem hiding this comment.
woops, I actually think I got these Trues and Falses backwards. We want to skip the visible checks but include the default checks so that _default are excluded from the returned parameters entirely (didn't notice it until I saw your comment explanation! 😬 )
There was a problem hiding this comment.
ok, so do we want to do it this way then? As this way buries the logic deeper, it might be difficult to parse. Even now I'm not sure: if check_visible=True, I'd interpret that to only take visible psets, which would exclude _default. Now you're saying that check_visible should be False? And then that check_default should be True? That seems counter-intuitive to me.
There was a problem hiding this comment.
Yes, it will be (marginally) cheaper, so I think since this runs often it is worth the explanatory comment and keeping the code as efficient as possible.
There was a problem hiding this comment.
the problem is that even the explanatory comment doesn't help me understand the logic. ;) But okay, I'll recommit with the fix and move on.
There was a problem hiding this comment.
check_visible checks whether a parameter is visible, and if it is not, excludes it from the results. check_default similarly checks whether a parameter has any tag (compute, component, dataset, etc) as _default, and if so, excludes it from the results. In this case you do NOT want parameters that are tagged default, so we need to use check_default=True (or skip the parameters with a continue). We are not worried about parameters that are not visible/relevant, so we can save some time by passing check_visible=False.
We'll have coverage whenever that is released... but I guess its unlikely this will be reintroduced, especially with the comment explaining the reason for using the default checks. |
Yep, it passes. Merge away. |
* Import from within source tree overly restrictive (#819) In order to avoid same-name module conflicts with the system module (such as io.py), phoebe shouldn't be imported from its own source tree. The original test eliminated the entire package tree, including all auxiliary directories, which is overly restrictive and it prevented pytest from running when phoebe was installed with `pip -e`. This PR fixes that by blocking imports from the root directory and from the root/phoebe tree (the actual sources). Closes #806. * update tests for changes to pytest (#820) * Fixing up all pytests The new pytest version raises an error instead of warning when tests return a value, and when assert is used incorrectly. All tests have been touched up to not have issues with the latest pytest. * Update f-string to be compatible with python 3.7 f-string format f'{var=}' has been introduced in python 3.8, and it caused the 3.7 tests to fail. This fixes that issue. --------- Co-authored-by: Andrej Prsa <[email protected]> * Dynamical RVs now avoid meshing (#823) * Dynamical RVs now avoid meshing Calling b.compute_pblums() built the mesh and treated dynamical RVs as mesh-dependent. This fixes that for both run_compute() and for direct compute_pblums(), compute_l3(), and compute_ld_coeffs bundle methods. A new function, b._datasets_that_require_meshing(), filters datasets that require a mesh, i.e. 'lc', 'lp' and flux-weighted 'rv'. Closes #812. * Generalized dataset filtering with b._datasets_where() * Adding backend exception to mesh computation Even if phoebe backend didn't need meshes for a particular dataset, other backends do. This is now handled in the `_datasets_where()` function. --------- Co-authored-by: Kyle Conroy <[email protected]> * run_checks_compute() bugfix (#837) * run_checks_compute() bugfix The run_checks_compute() function included the internal "_default" compute parameter set in checking the ck2004 model atmosphere table existence in the passband files. This caused the checks to fail even if ck2004 was never needed. * Incorporating @kecnry's comments into the PR (along with additional comments) * Fixing the if statement per @kecnry's instructions. * Fix treatment of distance for alternate backends (#832) * regression test * don't duplicate distance handling for alt backends * temporarily skip jktebop * use top-level default_binary in test * version and changelog for 2.4.13 release * update setup-python to v5 to avoid deprecation warning --------- Co-authored-by: Andrej Prsa <[email protected]> Co-authored-by: Andrej Prsa <[email protected]>
* Import from within source tree overly restrictive (#819) In order to avoid same-name module conflicts with the system module (such as io.py), phoebe shouldn't be imported from its own source tree. The original test eliminated the entire package tree, including all auxiliary directories, which is overly restrictive and it prevented pytest from running when phoebe was installed with `pip -e`. This PR fixes that by blocking imports from the root directory and from the root/phoebe tree (the actual sources). Closes #806. * update tests for changes to pytest (#820) * Fixing up all pytests The new pytest version raises an error instead of warning when tests return a value, and when assert is used incorrectly. All tests have been touched up to not have issues with the latest pytest. * Update f-string to be compatible with python 3.7 f-string format f'{var=}' has been introduced in python 3.8, and it caused the 3.7 tests to fail. This fixes that issue. --------- Co-authored-by: Andrej Prsa <[email protected]> * Dynamical RVs now avoid meshing (#823) * Dynamical RVs now avoid meshing Calling b.compute_pblums() built the mesh and treated dynamical RVs as mesh-dependent. This fixes that for both run_compute() and for direct compute_pblums(), compute_l3(), and compute_ld_coeffs bundle methods. A new function, b._datasets_that_require_meshing(), filters datasets that require a mesh, i.e. 'lc', 'lp' and flux-weighted 'rv'. Closes #812. * Generalized dataset filtering with b._datasets_where() * Adding backend exception to mesh computation Even if phoebe backend didn't need meshes for a particular dataset, other backends do. This is now handled in the `_datasets_where()` function. --------- Co-authored-by: Kyle Conroy <[email protected]> * run_checks_compute() bugfix (#837) * run_checks_compute() bugfix The run_checks_compute() function included the internal "_default" compute parameter set in checking the ck2004 model atmosphere table existence in the passband files. This caused the checks to fail even if ck2004 was never needed. * Incorporating @kecnry's comments into the PR (along with additional comments) * Fixing the if statement per @kecnry's instructions. * Fix treatment of distance for alternate backends (#832) * regression test * don't duplicate distance handling for alt backends * temporarily skip jktebop * use top-level default_binary in test * version and changelog for 2.4.13 release * update setup-python to v5 to avoid deprecation warning --------- Co-authored-by: Andrej Prsa <[email protected]> Co-authored-by: Andrej Prsa <[email protected]>
* Import from within source tree overly restrictive (#819) In order to avoid same-name module conflicts with the system module (such as io.py), phoebe shouldn't be imported from its own source tree. The original test eliminated the entire package tree, including all auxiliary directories, which is overly restrictive and it prevented pytest from running when phoebe was installed with `pip -e`. This PR fixes that by blocking imports from the root directory and from the root/phoebe tree (the actual sources). Closes #806. * update tests for changes to pytest (#820) * Fixing up all pytests The new pytest version raises an error instead of warning when tests return a value, and when assert is used incorrectly. All tests have been touched up to not have issues with the latest pytest. * Update f-string to be compatible with python 3.7 f-string format f'{var=}' has been introduced in python 3.8, and it caused the 3.7 tests to fail. This fixes that issue. --------- Co-authored-by: Andrej Prsa <[email protected]> * Dynamical RVs now avoid meshing (#823) * Dynamical RVs now avoid meshing Calling b.compute_pblums() built the mesh and treated dynamical RVs as mesh-dependent. This fixes that for both run_compute() and for direct compute_pblums(), compute_l3(), and compute_ld_coeffs bundle methods. A new function, b._datasets_that_require_meshing(), filters datasets that require a mesh, i.e. 'lc', 'lp' and flux-weighted 'rv'. Closes #812. * Generalized dataset filtering with b._datasets_where() * Adding backend exception to mesh computation Even if phoebe backend didn't need meshes for a particular dataset, other backends do. This is now handled in the `_datasets_where()` function. --------- Co-authored-by: Kyle Conroy <[email protected]> * run_checks_compute() bugfix (#837) * run_checks_compute() bugfix The run_checks_compute() function included the internal "_default" compute parameter set in checking the ck2004 model atmosphere table existence in the passband files. This caused the checks to fail even if ck2004 was never needed. * Incorporating @kecnry's comments into the PR (along with additional comments) * Fixing the if statement per @kecnry's instructions. * Fix treatment of distance for alternate backends (#832) * regression test * don't duplicate distance handling for alt backends * temporarily skip jktebop * use top-level default_binary in test * version and changelog for 2.4.13 release * update setup-python to v5 to avoid deprecation warning --------- Co-authored-by: Andrej Prsa <[email protected]> Co-authored-by: Andrej Prsa <[email protected]>
The run_checks_compute() function included the internal "_default" compute parameter set in checking the ck2004 model atmosphere table existence in the passband files. This caused the checks to fail even if ck2004 was never needed.