Closed ghost closed 2 years ago
I've moved the configuration into pyproject.toml
in the main branch, where you can do this:
[tool.poetry.dependencies]
requests = { git = "https://github.com/requests/requests.git" }
https://python-poetry.org/docs/dependency-specification/#git-dependencies
If you rebase feature/schism
onto main
, you'll get this change and will be able to add this line to the .toml
PySCHISM
has similar implementations for tides and other forcings. Should I create "duplicate" items in the coupled model driver or should I add a higher level abstraction for forcing types?
- By duplicate I mean to have
ADCIRCTidalForcingJSON
as well asPySCHISMTidalForcingJSON
, etc.- The main issue is that field types within each
*JSON
class is based on the specific ADCIRC classes, and I don't want to assume that PySCHISM is exactly the same, although they might be very similar
yes, it would probably be best to implement two children of TidalForcingJSON
, one for ADCIRC
and one for SCHISM
. 'TidalForcingJSON`, as the parent, could probably implement some common fields if there are any
- Some of
PySCHISM
operations (fetching NWM or GFS/HRRR/ERA5 data) requires internet access. But if I submit the generate job toslurm
then it won't be able to download. Have you had such an issue with ADCIRCPy or all of its required data is always offline?
I had a similar problem with the Hera processing nodes. I ended up modifying large parts of ADCIRCpy to be able to either call to the Internet or only use local files if specified, and that seems to have solved the issue.
To test this functionality I used pytest-socket
to disable Internet access for certain tests to ensure that it wouldn't try to access network resources if given a file: https://github.com/oceanmodeling/StormEvents/blob/15ef8efaaf664c6478110b6cc247766da943bcf1/tests/test_nhc.py#L225-L252
That's very useful information; I appreciate it. Thanks a lot!
Attention: Patch coverage is 59.01374%
with 507 lines
in your changes missing coverage. Please review.
Project coverage is 64.01%. Comparing base (
1f23f21
) to head (e380a85
). Report is 374 commits behind head on main.
:exclamation: There is a different number of reports uploaded between BASE (1f23f21) and HEAD (e380a85). Click for more details.
HEAD has 2 uploads less than BASE
| Flag | BASE (1f23f21) | HEAD (e380a85) | |------|------|------| |python3.9-Linux|2|0|
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@zacharyburnettNOAA do you still have access to your NOAA-email based Github account?
@zacharyburnett, this pull request is ready for review. Can you please take a look at the changes when you have time? I rebased (squash, reorder) my changes at the end so that the commits are cleaned up.
There are a couple of things I had to do to get all tests passing. I was wondering if you see any issues with these changes and if you have any suggestions other than removing the test from test matrix:
macos
(ADCIRC
tests) run into segmentation error on python-3.7
, so I removed 3.7
from matrix. I didn't know how to keep ubuntu
with python-3.7
but skip all tests for macos
with python-3.7
; linux
w/ 3.7
tests go through fine. Do you have a better way in mind?PySCHISM
, (local function definition) the macos
tests for PySCHISM
generation fail. Since this is a PySCHISM
issue and not CoulpedModelDriver
issue, I just skip the PySCHISM
test (using pytest.mark.skipif
) for macos
PySCHISM
relies on cfdm
package, but the package doesn't support Python 3.10
(3.x
). So I removed 3.x
from tests and also limited the support (in pyproject.toml
) to >=3.7, <3.10
.
coupledmodeldriver.generate
imports both generate.schism
and generate.adcirc
which in turn import pyschism
and adcircpy
, even if the test is for ADCIRC
model, the package fails to be imported as it always tries to import pyschism
as well and importing pyschism
results in cfdm
being loaded which fails on 3.10
.All the tests required for the pull request are passing now, except 3.7
and 3.x
that I had to remove. Given all above:
3.7
and 3.10
be required tests for future pulls?@zacharyburnett I figured out how to exclude an item from test matrix! I'll add back python 3.7
testing, but still there's nothing I can do for 3.x
/3.10
. I'll remove the latter from the test requirements for now.
I apologize, I meant to respond earlier about that but did not get to it, but I'm glad you figured it out!
the only thing is it looks like this will decrease code coverage, maybe you could add more tests for different combinations of SCHISM and forcings, but otherwise it looks great
Thank you for your feedback. I think compared to ADCIRC the coverage is not very different, but I'll take another look to see if there's any room for improvement. For now only tidal forcing support is added for SCHISM. I'll add all the other forcing combinations for schism in future feature branches and will add the tests as well.
@zacharyburnettNOAA, I have a couple of questions about adding
PySCHISM
. Please let me know if I need to create separate tickets for each to discuss on this repo:PySCHISM
is not published yet, how should I add dependency insetup.cfg
? Should I just assume users installPySCHISM
separately?PySCHISM
has similar implementations for tides and other forcings. Should I create "duplicate" items in the coupled model driver or should I add a higher level abstraction for forcing types?ADCIRCTidalForcingJSON
as well asPySCHISMTidalForcingJSON
, etc.*JSON
class is based on the specific ADCIRC classes, and I don't want to assume that PySCHISM is exactly the same, although they might be very similarPySCHISM
operations (fetching NWM or GFS/HRRR/ERA5 data) requires internet access. But if I submit the generate job toslurm
then it won't be able to download. Have you had such an issue with ADCIRCPy or all of its required data is always offline?