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

Add profiles for TUV-x #171

Merged
merged 14 commits into from
Jul 9, 2024
Merged

Add profiles for TUV-x #171

merged 14 commits into from
Jul 9, 2024

Conversation

mattldawson
Copy link
Collaborator

@mattldawson mattldawson commented Jun 27, 2024

Adds the ability to create profiles and profile maps through the MUSICA API for use with TUV-x. Also updates the grid and grid map, so that grid maps can be created through the API and grids that are added to a map, or grids that are retrieved from a map can be used to update the values of the grid in the map (instead of just changing a local copy of the grid).

closes #121

@mattldawson mattldawson marked this pull request as ready for review July 5, 2024 20:24
Copy link
Collaborator

@boulderdaze boulderdaze left a comment

Choose a reason for hiding this comment

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

Looks good! I left a few comments that are very trivial. I was unsure what profile means in TUV-X context and looked for the related issue. In your PR comments, did you mean the closing issue be #121?

include/musica/tuvx/profile.hpp Outdated Show resolved Hide resolved
src/tuvx/profile.cpp Outdated Show resolved Hide resolved
src/tuvx/profile_map.cpp Outdated Show resolved Hide resolved
fortran/test/fetch_content_integration/test_tuvx_api.F90 Outdated Show resolved Hide resolved
fortran/util.F90 Show resolved Hide resolved
src/tuvx/grid_map.cpp Outdated Show resolved Hide resolved
@mattldawson
Copy link
Collaborator Author

Looks good! I left a few comments that are very trivial. I was unsure what profile means in TUV-X context and looked for the related issue. In your PR comments, did you mean the closing issue be #121?

ah, yes! thanks for catching that!

Comment on lines +70 to +71
grid_map_ = nullptr;
owns_grid_map_ = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
grid_map_ = nullptr;
owns_grid_map_ = false;
grid_map_ = nullptr;
owns_grid_map_ = false;

Is owns_grid_map_ the same thing as grid_map_ being null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point. I think they were needed in an earlier iteration, but I agree they seem to be unnecessary now. will remove

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for the map classes (GridMap and ProfileMap) the owns_* flags are needed because they can reference maps owned by the TUV-x instance, but for Grid and Profile they aren't needed and have been removed.

Comment on lines 116 to 117
profile->profile_ = nullptr;
profile->owns_profile_ = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question here, is owns_profile_ necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed!

Copy link
Collaborator

@boulderdaze boulderdaze left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the comments!

@mattldawson mattldawson removed the request for review from sjsprecious July 9, 2024 15:26
@mattldawson mattldawson merged commit 583162b into main Jul 9, 2024
61 checks passed
@mattldawson mattldawson deleted the develop-121-tuvx-profiles branch July 9, 2024 15:26
K20shores added a commit that referenced this pull request Aug 7, 2024
* Start on test_micm_box_model.F90.

* Start on test_micm_box_model.F90.

* In test_micm_box_model, create micm_t solver.

* Initialize some variables.

* Use configs/analytical.

* Use configs/analytical.

* Added micm solve.

* Start on Chapter 2.

* Literal include for test_micm_box_model.

* Literal include json configs.

* Updated CMakeLists.txt.

* Add config file description.

* Tutorial Chapter 2.

* Tutorial Chapter 2.

* Minor formatting.

* Minor formatting.

* Added link to MICM docs, test program output.

* Start on tutorial CMakeLists.txt.

* Created MUSICA/tutorial subdir.

* Added MUSICA_INCLUDE_DIR and MUSICA_LIBRARY_DIR.

* Switch to MUSICA_INSTALL_DIR/include /lib.

* Include musicaConfig.cmake.

* Include addtional cmake files.

* Added Fortran to CMakeList LANGUAGES.

* Added target_link_directories and libraries.

* Added find_package(musica).

* Comment use_package(musica_fortran), not working.

* Added cmake usage comments.

* Restored find_package for musica and musica_fortran.

* Start on Tutorial Chapter 0 text.

* Added literal include of Ch 0 CMakeLists.txt.

* Chapter 0 intro.

* Tutorial Chapter 0.

* update cmake options

* set vector matrix dimension

* Use music-fortran instead of musica_fortran.

* Added test_micm_box_model executable.

* Updated test_micm_box_model with new micm solve.

* Added deallocate( micm ) in test_micm_box_model.

* Auto-format code using Clang-Format (#170)

Co-authored-by: GitHub Actions <actions@github.com>

* pypi version badge

* fair software checklist badge

* compilation flag for grid cells, vector matrix dimension

* c api for vector ordered matrix

* add vector matrix test

* update fortran api

* chage the build type

* update python wrapper

* resolve memory leak

* fix intel docker

* tuvx on

* micm git tag

* edit erorr

* opdate the option name for open mp

* remove open mp fortran in c tests

* edit comment

* Removed find_package lines in this version.

* replace unique ptr T to T

* replace tempalte with  auto

* Add profiles for TUV-x (#171)

* add profiles for TUV-x

* update TUV-x commit

* split tuvx source into separate headers and modules

* split TUV-x interface into separate modules

* allow grid creation

* fix grid updates

* update fortran tuvx wrapper

* address cross-compiler issue

* fix invalid free

* update actions

* fix grid map ownership problem

* finish profile and profile_map

* address review comments

* Auto-format code using Clang-Format

* add map header

* expose solver type enum to python

* back to template

* back to template

* enum for solver type

* enum solver type for fortran

* add fortran 2023 feature comment

* pass num grid cells to constructor

* code clean up

* space uniformly

* specify enabled languages in top level cmake

* comment out the tuvx openmp tests

* revert back

* revert back

* Added tutorial/demo.f90 as a unit test.

* Auto-format code using Clang-Format

* Set demo.f90 path with PROJECT_SOURCE_DIR.

* Added tutorial dir .dockerignore.

* add radiator wrapper

* working-on file of radiator header

* fix typos, and incomplete parts

* code clean up

* add radiator src

* fix bugs

* fix bugs

* Moved tutorial subdir to fortran/test/tutorial.

* Removed demo_c executable.

* Updated docs.

* fix bugs in c sources

* fix bugs in f90 sources

* fix bugs

* temp add radiator test

* fix bugs

* passed test

* incomplete test

* temp file for transfer

* working version

* fix test

* fix bugs

* fix fortran bugs

* add radiator fortran test

* test tag

* add fortran test

* add tests back:

* fix docker integration

* revert cmake

* clean up

* code cleanup

* code clean up

* revert back to main branch

* 159 create an action to publish to pypi when we make a release (#184)

* testing automated pypi release

* trying to appropriately name the wheels

* only need one version of python to build wheels for all other versions

* trying to get macos to build successfully

* excluding universal2 arch build for macos

* seeing if arm64 works

* trying to skip musllinux

* trying to choose linux architecture support

* trying to skip musllinux again

* adding intel based mac build

* trying to set the osx deployment target

* trying a different way

* that wasn't it

* trying to force a minimum of 10.15 for macos?

* setting action trigger

* undoing version change

* correcting project name

* Address review comments

* fix tuvx git tag

* Python error checking (#189)

* adding error checking, some debug statements for the python wrapper

* adding error checking to python, updating micm tag

* getting tests to pass

* correcting a fortran test

* newest tuvx commit

* I don't know what's wrong with nvidia

* Updating citation version number

---------

Co-authored-by: David Fillmore <fillmore@ucar.edu>
Co-authored-by: Jiwon Gim <jiwongim@ucar.edu>
Co-authored-by: David Fillmore <1524012+dwfncar@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: GitHub Actions <actions@github.com>
Co-authored-by: Matt Dawson <mattldawson@gmail.com>
Co-authored-by: Jiwon Gim <55209567+boulderdaze@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow TUV-x profile data to be updated through the API
4 participants