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

Modify Mag_Diople notebook #19

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

MetaronWang
Copy link
Contributor

Modify a notebook, repair some bug of the 3D-picture shown. Refactor the code let the note more humanize.

@MetaronWang MetaronWang self-assigned this Apr 4, 2019
@MetaronWang MetaronWang requested review from lheagy and thast April 4, 2019 03:04
@lheagy
Copy link
Member

lheagy commented Apr 4, 2019

👋 @fourndo, would you be willing to take a look at @wwzzyyzzrr's updates to the Mag dipole app?

@lheagy
Copy link
Member

lheagy commented Apr 14, 2019

👋 Hi @fourndo, @thast: would one of you be willing to review this pr?

Copy link
Member

@thast thast left a comment

Choose a reason for hiding this comment

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

Many thanks for this PR @wwzzyyzzrr . This is definitely an improvements over the existing notebook. I do not have much to say about the code. But There are a couple of things about the notebook we should discuss before merging:

  • I realize too that this notebook is not part of the index.ipynb. This PR could be the occasion to add it to it.

  • We have two Mag_Dipole_SOMETHING.ipynb now. It will be worth renaming them to highlight the differences between the two. Mag_DipoleApplet.ipynb seems to focus on profile while Mag_Dipole.ipynb focus on the field. Maybe we could rename them Mag_Dipole_Profile.ipynb and Mag_Dipole_Field.ipynb. If you have a better idea, I will be happy with it too.

  • There are tons of options in this notebooks. It is a bit overwhelming for the user. I think it comes down to "What is the purpose of this notebook?" and you do lack a section Purpose in it. So I would say:

    • Add a section 'Purpose': which questions are we trying to answer with this app?
    • Based on that, try to cut some of the options or display. Some suggestions: if we already have an
      app for profiles, maybe we do not need them here?
  • We lack a drawing to explain the parameters

  • Would it be possible to add an arrow in the 3D plot to represent the direction of the Earth inducing field?

  • Can you check the unit of the inducing field? The default value appears in the box as 5, which do not correspond to a realistic value in nT. I recommend using nT everywhere and thus a default value of 50,000.

  • Is having the field lines different colors necessary?

  • Many parameters describes in the text does not appear in the app such as but not limited to "Naz", "radii", "h", "profile_x", "profile_y"... to relate to comment above, it might not be necessary to add them to the already crowded app but then we do not need their description either

  • good job on the layout of the widgets. Would it be possible to keep the same with a mix of FloatText and Sliders. I think that for parameters such as X, Y, Z and angles Sliders are nicer both for users and developers (we can set minimum and maximum values and the user do not have to figure out the range of possible values accepted by the app).

  • Minor: could we highlight the parameters describe in the notebook in bold (example B0: the magnitude of the earth's field)

  • Minor 2: could we use the viridis colormap instead of jet? This is best practice.

@lheagy
Copy link
Member

lheagy commented Jun 10, 2019

👋 Hi @yangdikun and @wwzzyyzzrr: I just wanted to check in and see if you have had a chance to take a look at @thast's review comments? Please let us know if we can help clarify anything!

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.

3 participants