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

Updated exposure function call: enable temporal filtering #88

Merged
merged 16 commits into from
Aug 7, 2024

Conversation

erialC-P
Copy link
Collaborator

@erialC-P erialC-P commented Aug 5, 2024

This PR updates the exposure function to incorporate the ability to include temporal filtering in the exposure output/s. Incorporating this update into the intertidal repo is required to enable moving the exposure demo notebook into the dea_notebooks repository whilst ensuring enabled functionality is provided by the intertidal repository.

Changes and updates:

  • all updates to the function call default to None so theoretically, no updates should be required to the remainder of our production code base.
  • [Important] returned exposure values are now included in a dict object. A working prototype for handling this is included in the Intertidal_workflow.ipynb.
  • Optional inclusions in this new exposure function include the ability to apply temporal filters to the exposure high temporal resolution tidal modelling (e.g. winter dates only). Separately, tuples of filters can be applied (e.g. winter daytime). Lastly, functionality is built in to return the tide modelling and filtered tide height values to enable demonstration of the exposure method in other workflows.

Future work:

  • combine single and tuple filter options into single function call
  • thoroughly check all docstrings to ensure they are up to date

@erialC-P erialC-P requested a review from robbibt August 5, 2024 23:47
Removed a outstanding references to spatial filtering and ensured script is up to date and consistent with closed PR #85
erialC-P added a commit that referenced this pull request Aug 6, 2024
Copy link

github-actions bot commented Aug 6, 2024

For full integration test results, refer to the Tests directory README.

)

# Write each exposure output as new variables in the main dataset
for x in exposure_filters.data_vars:
Copy link
Member

Choose a reason for hiding this comment

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

I think given we're only ever going to be extracting the "unfiltered" layer here, I think we can simplify this code to:

ds["exposure"] = exposure_filters["unfiltered"]

intertidal/exposure.py Outdated Show resolved Hide resolved
# Calculate the tide-height difference between the elevation value and
# each percentile value per pixel
diff = abs(tide_cq - dem)
modelledtides_1d = modelledtides_lowres.mean(dim=["x", "y"])
Copy link
Member

Choose a reason for hiding this comment

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

There's a similar code block below this that is commented out - can you have a look and see if we can remove one of these?

intertidal/exposure.py Outdated Show resolved Hide resolved
intertidal/exposure.py Outdated Show resolved Hide resolved
# Calculate exposure
ds["exposure"], tide_cq = exposure(
exposure_filters, tide_cq_dict = exposure(
Copy link
Member

Choose a reason for hiding this comment

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

In the function below, tide_cq_dict is called modelledtides_ds - can we use that terminology here too for consistency?

(are these raw modelled tides or tide quantiles? Alternatively, we could call it something like tide_quantiles_ds too if that was more clear)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll update that variable name. It's a legacy hangover from an earlier version of the code.

erialC-P and others added 12 commits August 7, 2024 10:37
Co-authored-by: Robbi Bishop-Taylor <[email protected]>
Co-authored-by: Robbi Bishop-Taylor <[email protected]>
Co-authored-by: Robbi Bishop-Taylor <[email protected]>
updated:
- library import statements
- docstrings
- reviewer comments from PR #88
- applied Black formatting
- updated exposure func call in line with returned function variable names
- simplified the addition of 'unfiltered exposure' into master ds
updated exposure function call to align with returned variable names in function definition
@robbibt robbibt self-requested a review August 7, 2024 06:59
Copy link
Member

@robbibt robbibt left a comment

Choose a reason for hiding this comment

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

Wooo!

@erialC-P erialC-P merged commit d794b9f into develop Aug 7, 2024
@erialC-P erialC-P deleted the exposure_update branch August 7, 2024 07:01
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.

2 participants