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

Feature 47 diff index #57

Merged
merged 10 commits into from
Oct 23, 2020
Merged

Feature 47 diff index #57

merged 10 commits into from
Oct 23, 2020

Conversation

lindsayrblank
Copy link

@lindsayrblank lindsayrblank commented Oct 22, 2020

Pull Request Testing

  • Describe testing already performed for these changes:

    Transitioned the plotting of the difficulty index to METplotpy. Added plotting function, testing, expected output, test data, and documentation.

  • Recommend testing for the reviewer to perform, including the location of input datasets:

    Please check the code to make sure formatting is correct. Please run the following "pytest test_difficulty_index_plotting.py". You can look at the output images if you want, but all that matters is the test passing.

  • Will this PR result in changes to the test suite? [Yes or No]

    If yes, describe the new output and/or changes to the existing output:

    Yes, this will add an entire new test since the difficulty index is a new plotting functionality.

  • After merging, should the reviewer DELETE the feature branch from GitHub? [Yes or No]

    No, I will do that.

Pull Request Checklist

See the METplus Workflow for details.

  • Complete the PR definition above.
  • Ensure the PR title matches the feature or bugfix branch name.
  • Define the PR metadata, as permissions allow.
    Select: Reviewer(s), Project(s), and Milestone
  • After submitting the PR, select Linked Issues with the original issue number.

@lindsayrblank lindsayrblank added this to the METplotpy-1.0 milestone Oct 22, 2020
@lindsayrblank lindsayrblank self-assigned this Oct 22, 2020
@lindsayrblank lindsayrblank linked an issue Oct 22, 2020 that may be closed by this pull request
27 tasks
Copy link
Collaborator

@bikegeek bikegeek left a comment

Choose a reason for hiding this comment

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

I had to modify import statements and there is an assertion error when I run the test (when running within Pycharm). But when I run from command line and set the PYTHONPATH, it works as you originally set it. You might want to leave it as it is since Pycharm is probably doing something "under the covers"

from metcalcpy.calc_difficulty_index import forecast_difficulty as di
from metcalcpy.calc_difficulty_index import EPS
from metcalcpy.piecewise_linear import PiecewiseLinear as plin
import metplotpy.difficulty_index.mycolormaps as mcmap
Copy link
Collaborator

@bikegeek bikegeek Oct 22, 2020

Choose a reason for hiding this comment

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

This only works when I use the following import (when running with PyCharm IDE):
import difficulty_index.mycolormaps as mcmap

I suspect PyCharm is doing something "under the covers"

But when I run from command line and set the PYTHONPATH, it works as you originally set it. You can try leaving it as you originally have it.

from metcalcpy.calc_difficulty_index import EPS
from metcalcpy.piecewise_linear import PiecewiseLinear as plin
import metplotpy.difficulty_index.mycolormaps as mcmap
from metplotpy.difficulty_index.plot_difficulty_index import plot_field
Copy link
Collaborator

Choose a reason for hiding this comment

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

like above, I could only successfully run if I used this import:
from difficulty_index.plot_difficulty_index import plot_field

Copy link
Collaborator

Choose a reason for hiding this comment

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

But when I run from command line and set the PYTHONPATH, it works as you originally set it.

@bikegeek bikegeek marked this pull request as draft October 22, 2020 23:28
@bikegeek bikegeek marked this pull request as ready for review October 22, 2020 23:37
@bikegeek bikegeek marked this pull request as draft October 22, 2020 23:37
Copy link
Collaborator

@bikegeek bikegeek left a comment

Choose a reason for hiding this comment

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

The test is passing now that I am using the same version of Matplotlib base (3.2.1) and Matplotlib v 2.2.2 which is what was used to generate the original NRL plot.

@lindsayrblank lindsayrblank marked this pull request as ready for review October 23, 2020 21:44
@lindsayrblank lindsayrblank merged commit 9525130 into develop Oct 23, 2020
@lindsayrblank lindsayrblank deleted the feature_47_diff_index branch October 23, 2020 21:54
bikegeek added a commit that referenced this pull request Apr 27, 2024
…s to enable the plot to work with the current inheritance structure.
bikegeek added a commit that referenced this pull request Apr 27, 2024
bikegeek added a commit that referenced this pull request Apr 27, 2024
bikegeek added a commit that referenced this pull request Sep 30, 2024
bikegeek added a commit that referenced this pull request Oct 1, 2024
* Internal Issue #57 added logging support and changed names of settings to enable the plot to work with the current inheritance structure.

* Internal Issue #57 logging support

* Internal Issue #57 updates for logging and to work with current inheritance structure

* Replace printing to stdout with logging for both exceptions and warnings, and instead of logging when there is an error with reading the config file, exit.

* remove unused catch_warnings()

* Added logging and better error handling

* Added logging settings with description

* Remove unused logging assignment

* Make config file generic

* METplus-Internal #57 Added logging for the polar ice plot script

* METplus-Internal issue #57 modified logging for skew T to use common logger

* Added default config file to test

* Added the default config filename to constructor

* Fix import statement

* Address a SonarQube issue in make_maps() wrt logger

* Update metplotpy/plots/polar_plot/polar_ice.yaml

Co-authored-by: Julie Prestopnik <[email protected]>

---------

Co-authored-by: Julie Prestopnik <[email protected]>
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.

Add Difficulty Index plotting from NRL
2 participants