Add tides test case #456
Add tides test case #456
Conversation
|
@xylar, I still need to add the documentation pages, but opened this to start getting initial feedback. |
|
Here's an example of the M2 constituent plot generated at the end of the forward step:
|
|
The forward step took about a half hour to run on Compy with 5 nodes. |
|
Tested. Here are my commands, on badger: It downloaded some new files. Good thing we cleaned up disk space on IC! The setup of the mesh and init steps works. On the forward step I get this error. I'm not familiar enough with compass to debug this easily. forward setup error
|
|
The run for the mesh step was successful. Using an interactive node on badger: report:
but the run step for init was not. I am using the ocean executable from the head of E3SM/master. report:
Perhaps my compass version is getting mixed up somehow, since this must have worked for you? I redid my |
|
Yes, changing those both to report:
|
| """ | ||
|
|
||
| config = self.config | ||
| self.tpxo_version = config.getint('tides', 'tpxo_version') |
There was a problem hiding this comment.
| self.tpxo_version = config.getint('tides', 'tpxo_version') | |
| self.tpxo_version = config.get('tides', 'tpxo_version') |
This isn't an int.
| qpos = map_to_r3(mesh, xmid, ymid, head, tail) | ||
|
|
||
| ttic = time.time() | ||
| __, nloc = tree.query(qpos, n_jobs=-1) |
There was a problem hiding this comment.
| __, nloc = tree.query(qpos, n_jobs=-1) | |
| __, nloc = tree.query(qpos, workers=-1) |
based on https://docs.scipy.org/doc/scipy/reference/generated/scipy.spatial.cKDTree.query.html
Maybe this is a recent API change?
There was a problem hiding this comment.
Yes, apparently this is a scipy-1.8.0 change. Do you know if there's a way to maintain backward compatibility with this kind of thing?
There was a problem hiding this comment.
@dengwirda, we could do a try/except to first try the new interface. Or we could query for scipy version and select the arguments accordingly.
There was a problem hiding this comment.
For compass, I would suggest we just constrain scipy >=1.8.0. @sbrus89, can you add that?
https://github.com/MPAS-Dev/compass/blob/master/setup.py#L35
https://github.com/MPAS-Dev/compass/blob/master/conda/recipe/meta.yaml#L67
https://github.com/MPAS-Dev/compass/blob/master/conda/compass_env/spec-file.template#L30
There was a problem hiding this comment.
import scipy
print(scipy.__version__)should be able to tell the version of scipy. Then, something using:
import pkg_resources
import scipy
new_args = pkg_resources.parse_version(scipy.__version__) >= pkg_resources.parse_version('1.8.0')
if new_args:
__, nloc = tree.query(qpos, workers=-1)
else:
__, nloc = tree.query(qpos, n_jobs=-1)There was a problem hiding this comment.
@xylar - sure, I can. If we set the version in the conda environment, then do we also need the changes to select arguments based on version?
There was a problem hiding this comment.
I would just do the version constraint. The code snippet here is more for @dengwirda to use outside of compass if needed.
There was a problem hiding this comment.
Thanks @xylar, and I agree that for compass just supporting scipy >= 1.8.0 makes sense.
|
@sbrus89, just figured I'd look into the errors that @mark-petersen saw. I haven't had time to do a more thorough review yet but I hope to soon. |
|
Thanks @mark-petersen and @xylar. I'm surprised this worked for me at all with these issues hanging around. I fixed these problems in the most recent commit. |
|
I'm now getting url: https://web.lcrc.anl.gov/public/e3sm/mpas_standalonedata/mpas-ocean/tides/TPXO9/extract_HC is indeed a bad address. Looks like one of these lines: |
|
@mark-petersen - I have fixed the issue with the extract_HC path. |
|
@xylar, it looks like when the |
There was a problem hiding this comment.
@sbrus89 thank you, this is an impressive amount of work! This now sets up and runs the three tests (mesh, init, forward) to completion on badger, and creates the same plots as above.
|
@sbrus89, you can use |
| for con in self.constituents: | ||
| print('') | ||
| print(f'run {con}') | ||
| subprocess.call(f'./extract_HC < inputs/{con}_setup', shell=True) |
There was a problem hiding this comment.
Do a chmod right before this call, maybe.
There was a problem hiding this comment.
At the top, you'll need to add:
import statthen, here:
| subprocess.call(f'./extract_HC < inputs/{con}_setup', shell=True) | |
| os.chmod('extract_HC', stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR) | |
| subprocess.call(f'./extract_HC < inputs/{con}_setup', shell=True) |
There was a problem hiding this comment.
If extract_HC produces any output, we will need to figure out a way to redirect its output to self.logger (which points to a log file when running test suites). Otherwise, the output will be quite disruptive when running a suite.
There was a problem hiding this comment.
I believe adding this import:
from mpas_tools.logging import check_calland then changing the call to:
| subprocess.call(f'./extract_HC < inputs/{con}_setup', shell=True) | |
| check_call(f'./extract_HC < inputs/{con}_setup', logger=self.logger, shell=True) |
should work. We can find out. This check_call also logs the command being run by default so you don't need to do that above here.
|
I checked all the namelist and streams files against my run configurations. I found the following differences from the forward namelist: |
There was a problem hiding this comment.
@sbrus89, this looks amazing! You have done some very careful work here and it really shows what compass is capable of!
One think that you will need to remove from this PR is the update to the E3SM-Project submodule that happened in de6d7da. I tried running with the old submodule and that didn't work so it seems we do need to update E3SM-Project. But that will need to happen in a separate PR (e.g. #461) and should ideally happen before this gets merged.
| for con in self.constituents: | ||
| print('') | ||
| print(f'run {con}') | ||
| subprocess.call(f'./extract_HC < inputs/{con}_setup', shell=True) |
There was a problem hiding this comment.
At the top, you'll need to add:
import statthen, here:
| subprocess.call(f'./extract_HC < inputs/{con}_setup', shell=True) | |
| os.chmod('extract_HC', stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR) | |
| subprocess.call(f'./extract_HC < inputs/{con}_setup', shell=True) |
| for con in self.constituents: | ||
| print('') | ||
| print(f'run {con}') | ||
| subprocess.call(f'./extract_HC < inputs/{con}_setup', shell=True) |
There was a problem hiding this comment.
If extract_HC produces any output, we will need to figure out a way to redirect its output to self.logger (which points to a log file when running test suites). Otherwise, the output will be quite disruptive when running a suite.
| for con in self.constituents: | ||
| print('') | ||
| print(f'run {con}') | ||
| subprocess.call(f'./extract_HC < inputs/{con}_setup', shell=True) |
There was a problem hiding this comment.
I believe adding this import:
from mpas_tools.logging import check_calland then changing the call to:
| subprocess.call(f'./extract_HC < inputs/{con}_setup', shell=True) | |
| check_call(f'./extract_HC < inputs/{con}_setup', logger=self.logger, shell=True) |
should work. We can find out. This check_call also logs the command being run by default so you don't need to do that above here.
|
Running on Chsrysalis, I'm getting this error in Analysis: |
|
@knbarton thanks for your comment above: Those flags are set here: I think the three flags settings that Steve has in this PR are better.
|
|
Thanks @sbrus89 for the change! I'm trying to test again and am getting this traceback I must be missing this but where is the .cfg file? It seems it is looking for it here and can't find it. |
|
pulling the code again and re setting the cases fixed my error above. |
|
@vanroekel and @sbrus89, I was about to merge this -- it's had a lot of testing. But it seems like you're still doing some last checks. Feel free to merge when you're ready. @sbrus89, I think you have permission but if not I'll do it in the morning. |
|
@xylar one quick question it probably isn't related to this branch but came up when I was trying it out. I was in the initial_state part of the run and tried changing init_ntasks from 36 to 128 after the test was set up but then compass run still used 36 for tasks. Is it expected that if I change |
|
@vanroekel, it should change as you expected. We can fix that. |
|
Let's make a note of it in an issue for now. It relates to a method called |
|
sounds good to me. I'm able to run this test smoothly so am happy for this to get merged |
This PR adds the mesh and initial condition generation, forward model run, and analysis workflow for single layer tidal configurations. Currently, the Icosahedral 7 mesh is used as an initial resolution.