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

Added plotting function to check influence of flow parameters A and n #39

Merged
merged 13 commits into from
Dec 12, 2023

Conversation

vivekag7
Copy link
Contributor

@vivekag7 vivekag7 commented Nov 23, 2023

Note: only works with Sleipnir v0.4.0 + Muninn v0.2.6 + GlaThiDa update

Added plotting function to check influence of flow parameters A and n.

  • Supports a maximum grid size of 5x5 (lengths of A_values and n_values each should not exceed 5).
  • Only supports analysis for one glacier at a time (length of rgi_ids should be 1).

Usage of iceflow and mass balance can also be specified, according to your preference. The default is set to true.

See below an example plot:

image

Note that the colorbar is the same for each plot, which makes it easy to compare.

Copy link
Member

@JordiBolibar JordiBolibar left a comment

Choose a reason for hiding this comment

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

Good job, just a few comments to modify some little things. Mark them as solved and I will run the test and integrate it.

Project.toml Outdated Show resolved Hide resolved
src/Huginn.jl Outdated Show resolved Hide resolved
src/plotting/Plotting.jl Outdated Show resolved Hide resolved
src/plotting/plotting_utils.jl Show resolved Hide resolved
test/plotting.jl Outdated Show resolved Hide resolved
@vivekag7
Copy link
Contributor Author

vivekag7 commented Dec 8, 2023

The tests are still failing, however this should be fixed once Sleipnir is up to date with the recent changes and Sleipnir is updated within Huginn. Locally with the new Sleipnir all test pass

Copy link
Member

@JordiBolibar JordiBolibar left a comment

Choose a reason for hiding this comment

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

Hi @vivekag7. Please do these minor changes, and I will merge your PR and include it in the Huginn release I'll make today for ODINN's new architecture. Thanks!

src/Huginn.jl Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@vivekag7
Copy link
Contributor Author

Made another small change next to the requested fixes: I made sure the new plotting utility plot_analysis_flow_parameters() uses a Sleipnir.parameters object as an input. Several reasons why I did this:

  • aligns more with the use of other functions in this whole frame work.
  • readability of the function
  • more flexibility

Tested it locally and everything worked, so if everything is right it should work on github.

@vivekag7 vivekag7 reopened this Dec 12, 2023
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@171ad4f). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #39   +/-   ##
=======================================
  Coverage        ?   92.23%           
=======================================
  Files           ?       10           
  Lines           ?      399           
  Branches        ?        0           
=======================================
  Hits            ?      368           
  Misses          ?       31           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JordiBolibar JordiBolibar merged commit 2985440 into ODINN-SciML:main Dec 12, 2023
1 of 2 checks passed
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.

3 participants