Skip to content

Conversation

weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang weiyuan-jiang commented Jun 9, 2025

Initial check in. The class will be broken up into smaller steps.

This PR addresses issues #113 and #134.

Note: The ldas_setup changes impact #54 (irrigation), #131 (routing), and #132 (CatchCN51). Chances are that for each of these PRs, updating ldas_setup after the present PR has been merged will result in conflicts and require manual updates.

@gmao-rreichle
Copy link
Collaborator

@weiyuan-jiang : Many thanks for getting a start on this. Perhaps hold off on editing ldas_setup for refactoring until #102 and #106 have been merged. With any luck, we can merge these PRs early this week.

@gmao-rreichle
Copy link
Collaborator

@weiyuan-jiang : When you get back to this PR, please include an optional SLURM/PBS parameter ntasks-per-node, see issue #113. Note that unless the user specifies a value for ntasks-per-node, the default behavior should be to not specify an integer value for ntasks-per-node in the lenkf.j job file.
I don't know if it's possible to specify ntasks-per-node in lenkf.j with a blank value. If this works, I suppose we can add ntasks-per-node just like constraint. If a blank value for ntasks-per-node doesn't work in lenkf.j, we need a bit more scripting than what we do for constraint.

cc: @mathomp4

@gmao-rreichle gmao-rreichle linked an issue Jun 23, 2025 that may be closed by this pull request
@weiyuan-jiang
Copy link
Contributor Author

This PR addresses issue #134 and #113

@weiyuan-jiang weiyuan-jiang marked this pull request as ready for review July 10, 2025 12:45
@weiyuan-jiang weiyuan-jiang requested review from a team as code owners July 10, 2025 12:45
@weiyuan-jiang
Copy link
Contributor Author

@gmao-rreichle I can merge PR #132 to this PR. Should I do that ?

@weiyuan-jiang
Copy link
Contributor Author

@saraqzhang Would you please test this PR in your GEOSadas run? I expect zero-diff. Thanks

@saraqzhang
Copy link
Contributor

@weiyuan-jiang @gmao-rreichle I tested ldas_setup with ladas options. It needs one fix in setup_utils.py : add "from datetime import timedelta ". After the fix the experiments are set up correctly.

@gmao-rreichle
Copy link
Collaborator

@weiyuan-jiang @gmao-rreichle I tested ldas_setup with ladas options. It needs one fix in setup_utils.py : add "from datetime import timedelta ". After the fix the experiments are set up correctly.

Thanks, @saraqzhang, for taking look so quickly. @weiyuan-jiang, I added the import statement.

@gmao-rreichle gmao-rreichle merged commit f1e382c into develop Jul 11, 2025
8 of 9 checks passed
@gmao-rreichle gmao-rreichle deleted the feature/wjiang/cleanup_ldas_setup branch July 11, 2025 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GEOSldas should add ntasks-per-node as an optional input
3 participants