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

empty test observation coverage plot #146

Closed
wants to merge 2 commits into from

Conversation

Hendrik1987
Copy link
Contributor

@ecomodeller and @jsmariegaard , working on the long list of ideas #132 will likely fix this, but I need an ad-hoc solution now :-)

I can think of two approaches:

  1. Add fmskill.plot.plot_observation_coverage(), which can be called by the Connector or stand alone. This would be consistent with the other plot methods here, such as fmskill.plot.plot_observation_positions, which is called here.
  2. Allow a connection without a model result: Connector([o1, o2, o3], '')

I feel that 2 is more overlapping with a general rework, so my proposal for now would be option 1.

Thoughts? Other options?

@ecomodeller
Copy link
Member

The only way to get through a long list of tasks is to get started with the most pressing one...

@ecomodeller
Copy link
Member

Should be pretty straightforward to extract
a separate method from this https://github.com/DHI/fmskill/blob/3c0bae70210924e52d356b5b8259c2e243b8aa8c/fmskill/connection.py#L589 to only the temporal coverage to plot a list of observation.

Create a function that make sense to you!

@Hendrik1987
Copy link
Contributor Author

Thanks @ecomodeller .
Will give it a shot with mikeio.plot.plot_observation_periods() as sister to mikeio.plot.plot_observation_postitions().

@jsmariegaard
Copy link
Member

This is being addressed in #187 - I suggest closing this PR.

@jsmariegaard
Copy link
Member

Closed by #187

@jsmariegaard jsmariegaard deleted the observation-coverage-without-model-results branch November 20, 2023 08:42
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.

None yet

3 participants