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

Interactive plots #154

Merged
merged 28 commits into from
Nov 29, 2024
Merged

Interactive plots #154

merged 28 commits into from
Nov 29, 2024

Conversation

mjmroz
Copy link
Contributor

@mjmroz mjmroz commented Nov 13, 2024

I added an option for creating interactive plots for the best model using Plotly. An interactive plot will be created if the YAML file includes: 'plots': 'best model': 'interactive': file.html. Some settings for the interactive plot are inherited from the regular plot, while others are defined within ulens_model_fit.py. Although some new functions are quite long, I am concerned that splitting them into smaller ones would make adjusting interactive plot settings a non-trivial task for users.

@rpoleski
Copy link
Owner

Good to see first pass, which already looks quite good. I'll have a deeper look into it.
At this point:

  • In MM we try to limit functions to 20 lines of code and this approach is taken from people with more experience in coding. The positive aspect of that is that one is able to see whole function at once on the screen. Let's try to change the code you added that way.
  • It will be good to add a minimum example for such plots. You can do it or I'll try it myself (I can push to this branch, so please pull before you start coding).

@rpoleski
Copy link
Owner

  • What is the reason to force best model -> file to be defined if best model -> interactive is defined?
  • In MM we have the convention that higher-level functions are listed first, i.e., _make_interactive_plot() should be at line 3817.
  • I see you actually made a minimal example by modifying an existing file. It would be better to add a separate file.
  • I don't see errorbars in the main plot... OK, that was easy to correct.
  • Import of plotly should be moved to import_failed part.
  • If the second Y axis is not possible currently, then proper error message should be given.

@mjmroz
Copy link
Contributor Author

mjmroz commented Nov 17, 2024

I have split some longer functions. The longest function right now is _make_interactive_layout(), which simply involves passing many parameters to an object. To make it shorter, I could create a YAML file containing these parameters and read them from there, let me know if you prefer this option. I moved the plotly import to import_failed. I have also allowed only interactive plots. Currently, while only interactive plots are being plotted, a regular plot will still be created but not saved. This is necessary because _best_model_plot() sets certain parameters that are later needed for the interactive plot.

@mjmroz
Copy link
Contributor Author

mjmroz commented Nov 17, 2024

I also added a warning when the interactive plot and second Y axis is selected.

@rpoleski
Copy link
Owner

In 7d465f3 I've corrected order of functions.

While doing so, I've found that _make_interactive_scatter_lc_satellite() is not called at all.

@rpoleski
Copy link
Owner

In the last commit I've simplified the code by reducing duplicated statements. Please try to remove copy-pasted code in a similar way.

@rpoleski
Copy link
Owner

Also I see that from sizes[] only elements 4, 7, 8, and 9 are used. Am I missing something? Unused code should be removed.

@rpoleski
Copy link
Owner

Are you sure the return statements in _make_interactive_lc_traces() are correct? I would guess the first one should be replaced by break command.

@rpoleski
Copy link
Owner

Two more minor things:

  • version update :)
  • update of _check_imports()

@rpoleski
Copy link
Owner

Please see Semantic Versioning rules.

@rpoleski rpoleski merged commit bbd71e9 into rpoleski:master Nov 29, 2024
1 check 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.

2 participants