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

[1pt] PR: Convert CatFIM pipeline to open source #292

Merged
merged 13 commits into from
Mar 9, 2021

Conversation

BrianAvant
Copy link
Contributor

@BrianAvant BrianAvant commented Mar 3, 2021

Convert CatFIM pipeline to open source to resolve issue #279

Changes

  • added VIZ_PROJECTION to shared_variables.py
  • added missing library referenced in inundation.py
  • cleaned up and converted evaluation scripts in categorical FIM generation to open source
  • removed util folders under tools directory

@BrianAvant BrianAvant added bug Something isn't working enhancement New feature or request CatFIM NWS Flood Categorical HAND FIM labels Mar 3, 2021
@BrianAvant BrianAvant added this to the FIM v3.X milestone Mar 3, 2021
@BradfordBates-NOAA BradfordBates-NOAA changed the title PR: Convert CatFIM pipeline to open source [1pt] PR: Convert CatFIM pipeline to open source Mar 3, 2021
@BradfordBates-NOAA
Copy link
Member

BradfordBates-NOAA commented Mar 4, 2021

Hey Brian, here are some things I noticed upon initial review:

  • sys imports sys.path.insert need to account for Cahaba restructure--probably don't need the "tests" import because the relevant files are inside of tools alongside generate_categorical_fim.py
  • PREP_PROJECTION and VIZ_PROJECTION are not in tools/utils/shared_variables.py, both fail to import
  • I recommend maintaining PEP8 styling, with 2 returns before a function definition, also capitalized docstrings
  • There are some variables used that are not defined, e.g. log_file, log_dir in the reformat_inundation_maps function
  • The argparse description suggests that the script will do regression analysis, but this is out of scope for generate_categorical_fim.py

@BradfordBates-NOAA
Copy link
Member

BradfordBates-NOAA commented Mar 5, 2021

I confirmed that the script ran all the way through. Nice!

I have some feedback regarding the fields that I did not mention in previous comments:

  • The final layer needs the following fields:

    • q: Discharge for the respective flood category
    • stage: Stage for the respective flood category
    • q_src: Discharge source
    • stage_src: Stage source
    • q_uni: Discharge units
    • stage_uni: Stage units
  • Please round q and stage to the nearest decimal point

  • Please change WRDS_time to wrds_time

The values for the fields listed above can be derived from the existing fields that are available post-join (e.g. Q_act, S_min, etc.), however all fields need to essentially be "dissolved" into the fields in the list above. Here's a screenshot of the source data attribute table for the CatFIM service:

image

@BrianAvant
Copy link
Contributor Author

I confirmed that the script ran all the way through. Nice!

I have some feedback regarding the fields that I did not mention in previous comments:

* [ ]   The final layer needs the following fields:
  
  * `q`: Discharge for the respective flood category
  * `stage`: Stage for the respective flood category
  * `q_src`: Discharge source
  * `stage_src`: Stage source
  * `q_uni`: Discharge units
  * `stage_uni`: Stage units

* [ ]  Please round `q` and `stage` to the nearest decimal point

* [ ]  Please change `WRDS_time` to `wrds_time`

The values for the fields listed above can be derived from the existing fields that are available post-join (e.g. Q_act, S_min, etc.), however all fields need to essentially be "dissolved" into the fields in the list above. Here's a screenshot of the source data attribute table for the CatFIM service:

image

Almost all of these are related to the input table I am joining to the spatial layer. Seems like those changes should be made there and not in this workflow. I'll get with @TrevorGrout-NOAA about resolving these.

@BradfordBates-NOAA
Copy link
Member

BradfordBates-NOAA commented Mar 5, 2021

I think the fields in the list can only be created after the join because of the way the join table and the CatFIM library are formatted. We can chat more about it on a call though. But yes, the name of the WRDS_time could be handled by Trevor's file, as well as the significant digits.

@BrianAvant
Copy link
Contributor Author

Should be good to go. Trevor said he would deal with rounding in his feature branch.

@BradfordBates-NOAA
Copy link
Member

I ran it again and everything looks great, although we need to add WFO as an attribute. I've talked with @TrevorGrout-NOAA about this.

Copy link
Member

@BradfordBates-NOAA BradfordBates-NOAA left a comment

Choose a reason for hiding this comment

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

Approving. The WFO attribute will be represented after it is available in the necessary join table.

@BradfordBates-NOAA BradfordBates-NOAA merged commit 30f401a into dev Mar 9, 2021
@BradfordBates-NOAA BradfordBates-NOAA deleted the dev-catfim-workflow branch March 9, 2021 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CatFIM NWS Flood Categorical HAND FIM enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants