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

docs: update installation instructions for CAM with FTorch #19

Merged

Conversation

TomMelt
Copy link

@TomMelt TomMelt commented Oct 29, 2024

Hi @jatkinson1000 , here's my take on the build instructions.

There's no major differences. I just tried to align it with the steps I actually used to build the code. Biggest one is probably that I removed running module load libtorch because it conflicts with the base ncarenv/23.06 module, and we don't actually need to load it. I did add an explanation though as to why it's been removed.

One thing I realised in the process though is the description for the xml vars gw_convect_dp_ml_net_path and gw_convect_dp_ml_norms possibly needs updating. Am I right in thinking that even if gw_convect_dp_ml is set to .false., if gw_convect_dp_ml_compare is .true. do we not run both models anyway. For this README I took the definitions of the variables from bld/namelist_files/namelist_definition.xml.

Here's the relevant section from bld/namelist_files/namelist_definition.xml.

<entry id="gw_convect_dp_ml_norms" type="char*132" input_pathname="abs" category="gw_drag"
       group="gw_drag_nl" valid_values="" >
  Absolute filepath to the deep convection gravity wave normalisation weights (NetCDF)
  used when `gw_convect_dp_ml` is set to `.true.`.
</entry>

<entry id="gw_convect_dp_ml_compare" type="logical" category="gw_drag"
       group="gw_drag_nl" valid_values="" >
  Whether or not to run a piggybacking comparison of the ML deep convection gravity·
  waves to the original scheme. Only one scheme will be used to advance the simulation
  as dictated by `gw_convect_deep_ml`.
Default: .false.
</entry>

Let me know what you think 👍

(I wouldn't normally tag all of you in the review, I just thought you might like to see the build instructions. Probs just @j-emberton and @jatkinson1000 that need to approve)

@TomMelt TomMelt added the documentation Improvements or additions to documentation label Oct 29, 2024
@TomMelt TomMelt self-assigned this Oct 29, 2024
@TomMelt TomMelt requested a review from j-emberton October 29, 2024 11:14
README.md Show resolved Hide resolved
Copy link

@j-emberton j-emberton left a comment

Choose a reason for hiding this comment

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

This looks good Tom. The changes make sense and add to readability and comprehension me.

Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

Thanks @TomMelt
I have left a few suggestions/comments.

My overall impression after reading this is that the ### Obtaining FTorch section now feels quite empty. I wonder if there is some reshuffling that could make this easier to follow.

I'm happy for this to be my responsibility, as I realise this is a PR onto my PR, so the buck eventually stops with me.

I also note that on reading it is some time since I wrote these instructions (due to the delays in receiving a review (not from yourself!!)) and there are things I would do differently after working on the project for some time.
So, I'm keen to get this merged and then have a second pass through myself, so feel free to comment in #5 to make some of the points tasks for me to do.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

I think this is at a point where we should merge, and make further adjustments in the host PR.

Thanks @TomMelt

@jatkinson1000 jatkinson1000 merged commit 25204fa into datawave_ml-setup-docs Nov 5, 2024
@jatkinson1000 jatkinson1000 deleted the minor-changes-to-build-instructions branch November 5, 2024 08:28
jatkinson1000 added a commit that referenced this pull request Nov 25, 2024
* docs: update installation instructions for CAM with FTorch

* docs: update FTorch build instructions in README.md

Co-authored-by: Jack Atkinson <[email protected]>

* docs: fix typo in README.md

Co-authored-by: Jack Atkinson <[email protected]>

* docs: better explanation of `DOUT_S` xml parameter

Co-authored-by: Jack Atkinson <[email protected]>

* docs: correct explanation of setting `DOUT_S=FALSE`

Co-authored-by: Jack Atkinson <[email protected]>

* docs: better description of CAM version specifics

Co-authored-by: Jack Atkinson <[email protected]>

* docs: remove unnecessary info about libtorch module

* docs: add more description to the NN configuration

---------

Co-authored-by: Jack Atkinson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants