Skip to content

Adding the Compare to... time series options #333

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

Merged
merged 7 commits into from
May 23, 2023
Merged

Conversation

remicousin
Copy link
Contributor

This adds a 2nd time series to the local graph that puts the monitored one in context. Controls allow to compare different things (another planting date, another year, another crop).

There are a couple of problems.

I commented out the assertion in controls.py because it complained even though I really can't tell why it behaves differently from all the other Sentence objects. I printed that it believes the Number object for year_ago is a list instead of what it is asserting. There are Numbers in other Sentences... I don't get it...

The other problem is that the improper behavior of printing additional stuff first mentioned in #302 (and not addressed because we moved then to bar plots for which it's irrelevant) has returned. I get it each time I change the planting date month of the Compare to... ts from default June to May.

@remicousin remicousin requested review from aaron-kaplan and kgraaf May 3, 2023 20:39
@remicousin remicousin self-assigned this May 3, 2023
Copy link
Collaborator

@aaron-kaplan aaron-kaplan left a comment

Choose a reason for hiding this comment

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

I would prefer to see the code that's the same for the two plots factored out into a function or a loop rather than copy-pasted. Copy-paste mistakes are easy to make and often hard to notice.

I wonder if the missing controls I commented on below will fix the assertion. If not, let me know and I will investigate.

This plotly issue sounds similar to the problem you saw. It was fixed in plotly.js 2.17.1, which made it into plotly.py in version 5.12. We currently have plotly 5.11. Please try upgrading plotly (and everything else) by following the section "Adding or removing dependencies" in the README. (While you're at it, you could change the title to "Adding, removing, or upgrading dependencies.") Then create a new environment as described in the section "Creating a conda environment with this project's dependencies" (add that pointer to the "Adding..." section too), and test.

@remicousin
Copy link
Contributor Author

I would prefer to see the code that's the same for the two plots factored out into a function or a loop rather than copy-pasted. Copy-paste mistakes are easy to make and often hard to notice.

I wonder if the missing controls I commented on below will fix the assertion. If not, let me know and I will investigate.

This plotly issue sounds similar to the problem you saw. It was fixed in plotly.js 2.17.1, which made it into plotly.py in version 5.12. We currently have plotly 5.11. Please try upgrading plotly (and everything else) by following the section "Adding or removing dependencies" in the README. (While you're at it, you could change the title to "Adding, removing, or upgrading dependencies.") Then create a new environment as described in the section "Creating a conda environment with this project's dependencies" (add that pointer to the "Adding..." section too), and test.

While a new conda-lock is building... I have a couple questions.

So practically, code could potentially be factored in through the pgo.Scatter objects. Whether I go that far, or not, the question I have is how to deal with the trys/excepts that potentially return in the middle of the code an errorfig? Should the error checkings be inside the factored in function and the function returns an object or an errorfig (it would be weird that the function doesn't return objects of same nature always); or the function returns an object or an error and then follow up code returns the errorfig or keeps going.
I am sure there is best practice for this but I just don't know what it is.

See per my comment that there is no missing control/id. And the code fails with the Number object that comes after the DataNoYear object anyways...

@remicousin
Copy link
Contributor Author

Also... how to have italic or bold or what-have-you in the text in our f""" """ strings? When mentioning the name of something in the page like the control Compare to..., I would like to write it Compare to...
I read a bit about it yesterday but got lost through many pages with examples using f" " or markdown with """ """ (and never f""" """) and I figured I would ask in case it's trivial to you.

@aaron-kaplan
Copy link
Collaborator

Should the error checkings be inside the factored in function and the function returns an object or an errorfig (it would be weird that the function doesn't return objects of same nature always); or the function returns an object or an error and then follow up code returns the errorfig or keeps going.

I would keep display concerns (anything related to plotly) out of the calcuation function. Have it return a DataArray (?) on success; on failure, it could either return a sentinel value (e.g. None), or raise an exception that you catch in the calling function.

I don't know what difficulty you're having with italics. dcc.Markdown looks like an easy way to get it. " ", """ """, f" ", and f""" """ are all just ways of building a string. You can put markdown in any of them. Want to show me a particular example that didn't work?

@remicousin remicousin mentioned this pull request May 4, 2023
@aaron-kaplan
Copy link
Collaborator

After changing the month selector from June to May, I don't think I'm seeing the strange behavior with either version of the conda environment. Could you attach a screenshot? Here's mine.

image

@aaron-kaplan
Copy link
Collaborator

BTW I'm confused by the multiple submit buttons that all seem to do the same thing.

@aaron-kaplan
Copy link
Collaborator

I also uncommented the assertion you had commented out, and that assertion isn't failing either. Are you sure you don't have any uncommitted changes?

@remicousin
Copy link
Contributor Author

BTW I'm confused by the multiple submit buttons that all seem to do the same thing.

what is the confusion about more specifically?

@remicousin
Copy link
Contributor Author

I also uncommented the assertion you had commented out, and that assertion isn't failing either. Are you sure you don't have any uncommitted changes?

I do still get assertion errors... Actually had to comment one more...

@aaron-kaplan
Copy link
Collaborator

I still can't reproduce the assertion failure. Do you get it on initial page load, or only after changing certain inputs? If the latter, please give details.

@remicousin
Copy link
Contributor Author

I actually don't even get the app to run:

(enactsmulti) remic@shortfin01:/data/remic/Dash/python-maprooms/enacts/wat_bal (remic/compare)$ CONFIG=../config.yaml:../config-sng.yaml:../myconfig.yaml python maproom_monit.py
Traceback (most recent call last):
File "/data/remic/Dash/python-maprooms/enacts/wat_bal/maproom_monit.py", line 70, in
APP.layout = layout_monit.app_layout()
File "/data/remic/Dash/python-maprooms/enacts/wat_bal/layout_monit.py", line 66, in app_layout
controls_layout(
File "/data/remic/Dash/python-maprooms/enacts/wat_bal/layout_monit.py", line 362, in controls_layout
Sentence(
File "/data/remic/Dash/python-maprooms/enacts/wat_bal/controls.py", line 156, in Sentence
assert (isinstance(elems[-1], str) or isinstance(elems[-1], html.Span))
AssertionError
(enactsmulti) remic@shortfin01:/data/remic/Dash/python-maprooms/enacts/wat_bal (remic/compare)$

@aaron-kaplan
Copy link
Collaborator

aaron-kaplan commented May 10, 2023 via email

@remicousin remicousin requested a review from aaron-kaplan May 17, 2023 16:53
@aaron-kaplan
Copy link
Collaborator

I think the only thing still unresolved here is the issue of the multiple submit buttons, which I find problematic.

  • The buttons are in a box that scrolls, and they're never all visible at once, so unless you're already familiar with the interface you won't know that some of them are there. Sometimes the only button I can see is one above the control I'm changing, which doesn't submit the value of that control, but that's not obvious. The one that does submit the value of that control is below, scrolled out of sight.
  • All three buttons cause the graph to be regenerated, so it's not clear why we need three of them. I guess one changes the map and the chart while two only change the chart, so maybe there's a case to be made for having two, but even that might be more confusion than it's worth.

@remicousin
Copy link
Contributor Author

A few comments of different nature about this:

1/ You clearly haven't read the text describing the Maproom. It's also in a scrolling box and so it's also hard to see that there is more to read. Maybe we should change scrolling boxes with collapsable content?

2/ Regardless, a typical user will jump to the controls and not bother read text/instructions until they realize they don't understand what's going on, so the criticism remains valid.

3/ I personally feel those controls take too much space:
3a/ I would put the smaller ones (Pick a point, variable) in the top Control Bar (along with date) in a smaller shape
3b/ the bigger ones that make sense to put all together (Current Season and Compare to) are probably better where they are rather than in the top Control Bar, but I would work on their looks to be more compact and have a lot less white space. Hopefully that would require less of scrolling and possibly even give more room to text above.

4/ I believe those controls/buttons are in individual "Cards" objects that could have bolder or color coded frames so that they can be seen better.

Opinions?

@aaron-kaplan
Copy link
Collaborator

@kgraaf should get involved in this discussion since it has implications for what UI layout options we want in maprooms in general. (See Rémi's comment preceding this one.)

@remicousin do you want to move ahead with this version and improve the UI in subsequent PRs?

@remicousin
Copy link
Contributor Author

I've just pushed with what I was playing with, but happy to revert that. I am not very good at that business.

@aaron-kaplan
Copy link
Collaborator

My problem with the submit buttons is related to something I've mentioned before: for some reason the input boxes are much wider for me than they are for you, and as a result the form takes much more vertical space. Fixing that would probably make this a non-issue. See screenshot below.

Move ahead and address that later?

image

@remicousin
Copy link
Contributor Author

My problem with the submit buttons is related to something I've mentioned before: for some reason the input boxes are much wider for me than they are for you, and as a result the form takes much more vertical space. Fixing that would probably make this a non-issue. See screenshot below.

Move ahead and address that later?

image

It's funny that the behavior differ on your system vs mine... But anyway indeed we already have an issue #251 for this.

@remicousin remicousin merged commit 7868258 into master May 23, 2023
@remicousin remicousin deleted the remic/compare branch May 23, 2023 15:39
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