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

make_bcs output directory reorg #729

Merged
merged 49 commits into from
May 17, 2023
Merged

Conversation

biljanaorescanin
Copy link
Contributor

@biljanaorescanin biljanaorescanin commented Mar 20, 2023

Work towards new output structure for boundary conditions.
Removing "SMAP" from EASE grid cases. Changes were minimal since Weiyuan already prepared all the codes.

Requires first merging of #718.

cc: @gmao-rreichle @weiyuan-jiang

@biljanaorescanin biljanaorescanin added the 0 diff The changes in this pull request have verified to be zero-diff with the target branch. label Mar 20, 2023
@biljanaorescanin
Copy link
Contributor Author

biljanaorescanin commented Mar 20, 2023

I've tested few cases and all were zero diff. ( NLv3 C720, NLv5 M09, ICA C180 and one LatLon case for ICA. )
@gmao-rreichle let me know if you think I should run for more cases.

@gmao-rreichle gmao-rreichle added the Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) label Mar 20, 2023
@github-actions
Copy link

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: 0 diff, Contingent - DNA

@gmao-rreichle
Copy link
Contributor

Thanks, @biljanaorescanin. The changes look ok to me, but I think it would be better to put this branch on top of the branch associated with #718, and point the PR to that branch until #718 has been merged. Currently, there's some overlap between the PRs.
The present PR is meant to have make_bcs create the bcs files in the new dir tree structure, right?
If that's the case, then I think we should change the title of the PR to something like "make_bcs output directory reorg", and maybe change the title of #718 to something like "update make_bcs legacy input path.

@biljanaorescanin biljanaorescanin changed the title makebcs directory reorg make_bcs output directory reorg Mar 20, 2023
@github-actions
Copy link

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: 0 diff, Contingent - DNA

@biljanaorescanin biljanaorescanin changed the base branch from develop to feature/borescan_update_legacy_bcs_path March 20, 2023 23:25
@github-actions
Copy link

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: 0 diff, Contingent - DNA

@github-actions
Copy link

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: 0 diff, Contingent - DNA

@biljanaorescanin
Copy link
Contributor Author

@weiyuan-jiang we still have conflicts.

We should probably also change this in make_bcs and python codes, after email we all got:
#SBATCH --ntasks=28
#SBATCH --constraint=sky
to
#SBATCH --constraint="[cas|sky]" and use $SLURM_CPUS_ON_NODE

@weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang we still have conflicts.

We should probably also change this in make_bcs and python codes, after email we all got: #SBATCH --ntasks=28 #SBATCH --constraint=sky to #SBATCH --constraint="[cas|sky]" and use $SLURM_CPUS_ON_NODE

I have not finished my change on python program. You can go ahead change your make_bcs script, I will follow

@weiyuan-jiang
Copy link
Contributor

@biljanaorescanin , Have you tried to submitted many jobs? I have concern over this line

It appears at lat-lon, cubed-sphere and EASE cases. If some jobs are submitted simultaneously, this directory may be deleted by the other job that finishes earlier.

@weiyuan-jiang
Copy link
Contributor

Now make_bcs.py should be in sync with make_bcs. The three program make_latlon_bcs.py, make_cube_bcs.py and make_ease_bcs.py can be tested independently

@biljanaorescanin
Copy link
Contributor Author

@biljanaorescanin , Have you tried to submitted many jobs? I have concern over this line

It appears at lat-lon, cubed-sphere and EASE cases. If some jobs are submitted simultaneously, this directory may be deleted by the other job that finishes earlier.

I would have to try again. I did run before and there was no issues. But it has been a while so I don't know for sure.

@biljanaorescanin
Copy link
Contributor Author

Looks like running CF and LatLon cases is OK in all combinations, for many resolutions at the time, but any experiment with EASE has to be run separately just EASE.
This part we added for version:
Options for EASE grid version (land-only):
EASEv1
EASEv2 (default)

Gets appended anytime we have EASE in combination... so I get for example this mess if I do:
Land BCs version: ICA
Atmospheric resolution: b c12 M36
EASE grid version: EASEv2
Ocean resolution: O1
SMAP_EASEv2_M36.j
SMAP_EASEv2_c12.j
SMAP_EASEv2_b.j

I see during testing I didn't run many combinations at the time but individual and if I did test many cases they were in same case type CF, EASE or LatLon. So that OUTDIR you asked about is not the problem but EASE grid version being appended.

@gmao-rreichle
Copy link
Contributor

Looks like running CF and LatLon cases is OK in all combinations, for many resolutions at the time, but any experiment with EASE has to be run separately just EASE.

@biljanaorescanin, @weiyuan-jiang: For make_bcs only, maybe we can just add augment the following comment:

echo "Enter alphanumeric code(s) for atmospheric horizontal resolution from the above list"

with something like "Do NOT specify a mix EASE grid and cs/LatLon grid resolutions."

This should be enough of a fix for make_bcs until it is deprecated. I assume the same problem does not occur in the python version of make_bcs.

Does this make sense?

@weiyuan-jiang
Copy link
Contributor

Looks like running CF and LatLon cases is OK in all combinations, for many resolutions at the time, but any experiment with EASE has to be run separately just EASE.

I still think we should remove that line. The OUTDIR is shared across all the jobs. You got lucky if the jobs are executed sequentially.

@biljanaorescanin
Copy link
Contributor Author

Looks like running CF and LatLon cases is OK in all combinations, for many resolutions at the time, but any experiment with EASE has to be run separately just EASE.

I still think we should remove that line. The OUTDIR is shared across all the jobs. You got lucky if the jobs are executed sequentially.

We should then remove it for python. Let me know when it is ready so I can start testing.

Copy link
Contributor

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

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

approving for surface-preproc-team

@gmao-rreichle gmao-rreichle marked this pull request as ready for review May 8, 2023 14:19
@gmao-rreichle gmao-rreichle requested review from a team as code owners May 8, 2023 14:19
tclune
tclune previously approved these changes May 8, 2023
Copy link
Collaborator

@tclune tclune left a comment

Choose a reason for hiding this comment

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

CMake changes ok.

Glob be dangerous ...

…/Utils/Raster/makebcs/CMakeLists.txt

Co-authored-by: Matthew Thompson <matthew.thompson@nasa.gov>
@github-actions
Copy link

github-actions bot commented May 8, 2023

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: 0 diff, Contingent - DNA

gmao-rreichle
gmao-rreichle previously approved these changes May 8, 2023
Copy link
Contributor

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

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

I accepted & merged Matt's suggestion and re-approved the PR

@gmao-rreichle gmao-rreichle changed the base branch from feature/borescan_update_legacy_bcs_path to develop May 10, 2023 14:20
@gmao-rreichle gmao-rreichle dismissed stale reviews from tclune and themself May 10, 2023 14:20

The base branch was changed.

@gmao-rreichle gmao-rreichle requested a review from a team as a code owner May 10, 2023 14:20
@github-actions
Copy link

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: 0 diff, Contingent - DNA

1 similar comment
@github-actions
Copy link

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: 0 diff, Contingent - DNA

@github-actions
Copy link

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: 0 diff, Contingent - DNA

@github-actions
Copy link

Label error. Requires exactly 0 of: Contingent - DNA, Needs Lead Approval, Contingent -- Do Not Approve. Found: 0 diff, Contingent - DNA

@sdrabenh sdrabenh removed the Contingent - DNA These changes are contingent on other PRs (DNA=do not approve) label May 17, 2023
@mathomp4 mathomp4 self-requested a review May 17, 2023 14:31
@sdrabenh sdrabenh merged commit 1b4a86d into develop May 17, 2023
4 checks passed
@sdrabenh sdrabenh deleted the feature/borescan_restructure_bcs branch May 17, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff The changes in this pull request have verified to be zero-diff with the target branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants