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

teca_detect_node: porting the DetectNodes function from TempestExtremes to TECA #745

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

amandasd
Copy link
Collaborator

@amandasd amandasd commented Mar 8, 2023

Porting the DetectNodes function from TempestExtremes to TECA

@amandasd amandasd linked an issue Mar 8, 2023 that may be closed by this pull request
@amandasd amandasd force-pushed the 656-gpu-capable-tc-detector branch 6 times, most recently from ca18cc5 to 970c9a3 Compare March 11, 2023 00:31
@amandasd amandasd requested a review from burlen March 23, 2023 22:52
Copy link
Collaborator

@burlen burlen left a comment

Choose a reason for hiding this comment

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

I am so pleased with the progress you've made!!!

the user facing variables (those that are exposed via teca algorithm properties) should use snake case, the individual words are separated by '_'. so something like 'thelongvariablename' would become 'the_long_variable_name'. this is for consistency with the rest of teca.

Please factor string parsing code out of the get_upstream_request. If it is the case that the various strings need only be parsed once per run then the results, such as the list of arrays to request, can be cached for reuse rather than being generated as each time step is processed. Note: teca's algorithm properties are designed to handle situations like this where cached data depends on a number of properties. when any property is changed cached data can be cleared forcing a regeneration. You'd override teca_algorithm::set_modified to clear the cached data. The presence of cached values would be tested in get_output_metadata and generated on the first invocation or reused on subsequent invocations.

Before we can merge this we need to factor all of tempest code into it's own library. Let's call this library "TE lite". TE lite will have a CMake based build system and only will compile the sources we make use of, for example src/base and src/nodes into a library. TE lite will export this library and teca cmake will use it by find_package(TELight), if the library is not found the new teca algorithms relying on it will not be built.

TE Lite will not be invoking the tempest's existing makefile based build system from cmake, and instead rely on cmake to compile the library. hint: you can use teca's own cmake library generation code as a template. TE lite will not compile any of the tempest executables. The compiled TE lite library is only to include sources we call directly. It's OK to leave other tempest codes in the repo, but those will not be compiled into the library, and will likely be removed at some point down the line once this project is finished.

The TE lite library will publicly be hosted on the LBL-EESA github and will be a means of us and others, including tempest authors, obtaining and making use of the modifications and improvements we are making should the y be interested in doing so.

Tempest sources have a copyright block at the top of each file. Please add a line at the end of each copy right block in each file that you modify, including the date, your name, and a few words to a sentence or two documenting the modification. It doesn't have to be very detailed. Please make a pass now, to capture what's been done to now and going forward do this incrementally as you work.

Thank you!

@amandasd amandasd force-pushed the 656-gpu-capable-tc-detector branch 3 times, most recently from 9b91c9f to bd4eaa8 Compare April 3, 2023 22:37
@amandasd amandasd requested a review from burlen April 4, 2023 17:05
Copy link
Collaborator

@burlen burlen left a comment

Choose a reason for hiding this comment

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

Hi! Nice work.

I downloaded and tried it locally and made a few improvements to the cmake/build and regression testing. I made to TELite library itself, so you'll have to fetch those. I flagged a few things in the review, which I realize that you probably inherited.

Would like to see if we can produce cyclone tracks. Toward that end. Here is a tests dataset that has 1 cyclone of each Saphir-Simpson category from tropical storm up to cat 5. In /pscratch/sd/l/loring/tc_test_data/ on Perlmutter.

Would like to see a regression test for the detect_nodes class.

We may want to think about about factor all the inherited code into the TELite library, into a templated function that we call from teca_detect_nodes. We can talk about at the next meeting.

Thank you!

alg/teca_detect_nodes.cxx Outdated Show resolved Hide resolved
alg/teca_detect_nodes.cxx Show resolved Hide resolved
alg/teca_detect_nodes.cxx Show resolved Hide resolved
alg/teca_detect_nodes.cxx Outdated Show resolved Hide resolved
alg/teca_detect_nodes.cxx Outdated Show resolved Hide resolved
apps/teca_detect_nodes.cpp Outdated Show resolved Hide resolved
${launcher} ${app_prefix}/teca_detect_nodes \
--input_regex "${data_root}/test_tc_candidates_1990_07_0[12]\.nc" \
--sea_level_pressure PSL --500mb_height Z1000 --300mb_height Z200 \
--surface_wind_u UBOT --surface_wind_v VBOT
Copy link
Collaborator

Choose a reason for hiding this comment

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

it would be good to test the output of the detector

alg/teca_detect_nodes.cxx Outdated Show resolved Hide resolved
alg/teca_detect_nodes.cxx Outdated Show resolved Hide resolved
alg/teca_detect_nodes.cxx Outdated Show resolved Hide resolved
@burlen burlen linked an issue May 15, 2023 that may be closed by this pull request
@amandasd amandasd force-pushed the 656-gpu-capable-tc-detector branch from 64ba52a to 59b648f Compare August 20, 2024 23:04
@amandasd amandasd force-pushed the 656-gpu-capable-tc-detector branch 5 times, most recently from e5f2ba3 to 0a1c79e Compare September 28, 2024 03:32
@amandasd amandasd force-pushed the 656-gpu-capable-tc-detector branch 4 times, most recently from cfee221 to 1e49574 Compare October 29, 2024 00:19
@amandasd amandasd requested a review from taobrienlbl October 29, 2024 01:26
@amandasd amandasd force-pushed the 656-gpu-capable-tc-detector branch from 1e49574 to bf3a0ec Compare October 29, 2024 01:46
           testing improvements for TELite features
           use a specific version via travis.yml
           require the library to be found
           output prints
           adding TELite installation
           removed the two executions with REQUIRE_NETCDF_MPI=FALSE
           updated TECA_DATA_REVISION
           updated TELite commit
@amandasd amandasd force-pushed the 656-gpu-capable-tc-detector branch from bf3a0ec to 7188ecb Compare October 29, 2024 04:06
@amandasd amandasd force-pushed the 656-gpu-capable-tc-detector branch from 7188ecb to e44ff83 Compare October 29, 2024 04:35
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest renaming to teca_tempest_tc_detect.cpp since this just implements the TC detector app as we discussed on 11/5/2024

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.

develop a pipeline join. GPU capable TC detector
3 participants