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

Updates for the Landau_damping notebook #21

Closed
4 tasks
StanczakDominik opened this issue Apr 26, 2020 · 7 comments · Fixed by #35
Closed
4 tasks

Updates for the Landau_damping notebook #21

StanczakDominik opened this issue Apr 26, 2020 · 7 comments · Fixed by #35

Comments

@StanczakDominik
Copy link
Contributor

StanczakDominik commented Apr 26, 2020

The notebook needs a few updates:

  • The "add .. to sys.path" trick at the start made my import fail. It would also fail with the notebook in any other location. I think if the setup.py install or pip install of the package goes through well, it should be unnecessary.
  • The from vlapy import needs to be replaced with from vlapy.core.
  • The notebook - especially the "Initialize and run" part - could definitely use some more breaking up to explain the defined variables, especially the commented out sets of those. Drop some more narrative in!
  • A question, because the docs weren't really clear on that - what's the connection between the notebook and the MLFlow style run? does the notebook implement the same sort of simulation?
  • A stretch goal - I would have loved an animation to go along with the 2D plot (xarray.interactive may soon be an option to do that). That's completely optional, though.
@joglekara
Copy link
Owner

joglekara commented Apr 26, 2020

Thanks for the feedback here.

This notebook is quite a relic at this point. I made it early on in an effort to clarify the API but it's beyond needing a simple update or two, as you rightfully have pointed out.

Here's a slightly more updated notebook but still needs to be brought up to full speed:
https://github.com/joglekara/VlaPy/tree/ld_notebook_mlflow/notebooks

I'd be happy to have a contributor on it. I also wouldn't be opposed to removing it.

@StanczakDominik
Copy link
Contributor Author

The other notebook you've linked is really lovely in terms of displaying the current API! I think taking the explanations from the first one and adding some explanations for each of the physics parameters makes this a great downloadable introduction.

I'll try to find something in the next few days to procrastinate away by adding nbsphinx to the docs 😆

@StanczakDominik
Copy link
Contributor Author

Plasmapy's PR 792 is still stalled on review, but I think I could quickly add nbsphinx here, if you're down for that 😃

@joglekara
Copy link
Owner

@StanczakDominik let's do it!

@joglekara joglekara linked a pull request May 25, 2020 that will close this issue
@StanczakDominik
Copy link
Contributor Author

I merged nbsphinx into plasmapy yesterday; now that I got a second opinion there that I'm not doing anything wildly unreasonable, I feel more comfortable adding it here. I'll get it done by tomorrow - likely today.

@StanczakDominik
Copy link
Contributor Author

Hey, re: nbsphinx, I've been thinking about it a bit more and realized that the execute-notebook-at-doc-build part is not going to play nicely with MLFlow having to be launched before execution - its only use would be to include the already built notebook, but there's strong risk of the notebook diverging from the main API.

@joglekara
Copy link
Owner

ah, np. Regardless, thanks for looking into it @StanczakDominik !

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 a pull request may close this issue.

2 participants