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

WIP: Learning notebooks #8

Closed
wants to merge 18 commits into from

Conversation

icweaver
Copy link
Member

@icweaver icweaver commented Feb 17, 2021

@codecov
Copy link

codecov bot commented Feb 17, 2021

Codecov Report

Merging #8 (bee551c) into main (262e2d8) will decrease coverage by 0.24%.
The diff coverage is n/a.

❗ Current head bee551c differs from pull request most recent head 1942230. Consider uploading reports for the commit 1942230 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main       #8      +/-   ##
==========================================
- Coverage   91.00%   90.75%   -0.25%     
==========================================
  Files          12       12              
  Lines        1012      963      -49     
==========================================
- Hits          921      874      -47     
+ Misses         91       89       -2     
Impacted Files Coverage Δ
src/polynomial/poly-grads.jl 86.11% <0.00%> (-0.69%) ⬇️
src/integrated.jl 93.33% <0.00%> (-0.42%) ⬇️
src/polynomial/quad-grads.jl 91.66% <0.00%> (-0.30%) ⬇️
src/polynomial/poly.jl 94.61% <0.00%> (-0.25%) ⬇️
src/polynomial/elliptic.jl 88.80% <0.00%> (-0.18%) ⬇️
src/polynomial/series.jl 87.39% <0.00%> (-0.02%) ⬇️
src/secondary.jl 100.00% <0.00%> (ø)
src/orbits/simple.jl 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 262e2d8...1942230. Read the comment docs.

@mileslucas
Copy link
Member

Thanks @icweaver this looks like a great start. I think a first order of business will be to implement some less verbose show methods- I'll get on that tonight or tomorrow!

Something you might play with is a little interactive slider for radius instead of evaluating distinct curves.

@icweaver
Copy link
Member Author

Always down to show off PlutoUI

@mileslucas
Copy link
Member

after you rebase on #12 the printing should be a lot cleaner.

@icweaver
Copy link
Member Author

I like it!

@icweaver icweaver closed this Feb 22, 2021
@icweaver icweaver deleted the learning_notebooks branch February 22, 2021 08:25
@icweaver icweaver restored the learning_notebooks branch February 22, 2021 08:27
@icweaver
Copy link
Member Author

Whoops, didn't mean to close this. I guess trying to rename a branch that is being PRed auto-closes the original?

@icweaver icweaver reopened this Feb 22, 2021
@icweaver
Copy link
Member Author

icweaver commented Feb 22, 2021

Ok, should be rebased on top of your current changes now and ready to merge. It's looking like we're going to need some more learning notebooks pretty soon!

Copy link
Member

@mileslucas mileslucas left a comment

Choose a reason for hiding this comment

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

Overall looking pretty good. For these notebooks we can focus a lot more on practical steps (a la the documentation system) so explaining how to do things and catching common hiccups (like broadcasting) will make future docs even better.

Besides the content, we need to figure out how to actually get this into the documentation. Obviously we can't host a live server, so we need to find a way to incorporate this into the package docs with a small blurb about how to use it as a Pluto notebook.

Comment on lines 44 to 55
# ╔═╡ 13e902e0-70bd-11eb-00a2-698092b29b2c
md"""
We will now create a `SimpleOrbit` object, which is a subtype of `AbstractOrbit`. This type accepts the following fields:
"""

# ╔═╡ db44287e-70bd-11eb-31ea-a1130ca7077f
fieldnames(SimpleOrbit)

# ╔═╡ f80ed7c2-70bf-11eb-074a-e94f706e5c3b
md"""
Each field is already defined with default values, so we will go ahead and create our `SimpleOrbit` object with a period of 3 days and transit duration of 1 hour, which can be specified seamlessly with the external `Unitful.jl` package:
"""
Copy link
Member

Choose a reason for hiding this comment

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

some of the fieldnames for SimpleOrbit aren't really user-facing, like half_period, for example. So I think putting it in documentation might be confusing.

Perhaps another approach could be saying
"First we need to define some orbital parameters" -> "this is facilitated by any of the AbstractOrbits..." -> "SimpleOrbit has the most straightforward ... " -> "let's figure out how we can create a SimpleOrbit by pulling up the docs ?SimpleOrbit (or via the docs in the bottom right of the Pluto notebook...)" -> "now let's create an orbit with these parameters..."

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Comment on lines 65 to 66
We can now use this as input into a limb darkening law, which we define next:
"""
Copy link
Member

Choose a reason for hiding this comment

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

Would like to see more explaining what u is- "these are the coefficients used for the higher order (>0) terms of our limb-darkening law" -> "you'll notice the law we declare has one more term than we gave it; that is because there is always a constant term defined to be -1". some stuff like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Comment on lines 76 to 81
For this example, we are using a Polynomial limb darkening law from [Agol, Luger, Foreman-Mackey (2020)](https://ui.adsabs.harvard.edu/abs/2020AJ....159..123A/abstract). This law is part of the `AbstractLimbDark` supertype, which includes the following laws as well that we will explore in other notebooks:
"""

# ╔═╡ 832cd21e-70c5-11eb-05b5-cd9a588751fc
subtypes(Transits.AbstractLimbDark)

Copy link
Member

@mileslucas mileslucas Feb 22, 2021

Choose a reason for hiding this comment

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

Suggested change
For this example, we are using a Polynomial limb darkening law from [Agol, Luger, Foreman-Mackey (2020)](https://ui.adsabs.harvard.edu/abs/2020AJ....159..123A/abstract). This law is part of the `AbstractLimbDark` supertype, which includes the following laws as well that we will explore in other notebooks:
"""
# ╔═╡ 832cd21e-70c5-11eb-05b5-cd9a588751fc
subtypes(Transits.AbstractLimbDark)
For this example, we are using a polynomial limb darkening law from [Agol, Luger, Foreman-Mackey (2020)](https://ui.adsabs.harvard.edu/abs/2020AJ....159..123A/abstract). This law is uses analytically derived integrals which makes it very fast and numerically accurate. There are other limb darkening laws which share a similar `AbstractLimbDark` interface. This interface lets us create composite laws like `IntegratedLimbDark` or `SecondaryLimbDark` with almost no effort. We will explore these laws and more in further notebooks.
"""
# ╔═╡ 832cd21e-70c5-11eb-05b5-cd9a588751fc
subtypes(Transits.AbstractLimbDark)

and I would remove the cell with the subtypes method.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Comment on lines 143 to 144
md"""
Technically we are passing this to the `compute` function, which is aliased to `ld` for convenience. A row-vector of planet-to-star ratios can be passed as well, and the required light curve computations will be automatically broadcasted:
Copy link
Member

@mileslucas mileslucas Feb 22, 2021

Choose a reason for hiding this comment

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

Suggested change
md"""
Technically we are passing this to the `compute` function, which is aliased to `ld` for convenience. A row-vector of planet-to-star ratios can be passed as well, and the required light curve computations will be automatically broadcasted:
md"""
All of the limb darkening laws can be called like a function `ld(b, r)` or using the `compute` method- `compute(ld, b, r)`. We provide both for convenience- it's natural and easy to treat the laws as functions but some code is easier to write using the concrete `compute` method. In addition, we don't provide any default vectorized versions of the methods, since Julia does not need to be vectorized for speed (unlike numpy). This lets users apply our laws in a variety of ways (e.g., `map`, `broadcast`, `Threads.@threads`, etc.). Below we use [broadcasting](https://docs.julialang.org/en/v1/manual/arrays/#Broadcasting) with the `@.` macro to create a grid of outputs across the `t` and `rprs` column and row vectors.

Want to avoid causing undue confusion between compute and the "functor" style. Also, might be a little more gentle introducing broadcasting- for people coming from python they'll have no idea why everything doesn't just vectorize automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@icweaver
Copy link
Member Author

Overall looking pretty good. For these notebooks we can focus a lot more on practical steps (a la the documentation system) so explaining how to do things and catching common hiccups (like broadcasting) will make future docs even better.

Sounds good, I like the emphasis on pedagogy! Re-working it with that in mind now

@icweaver
Copy link
Member Author

Ok, changes should be up! (notebook preview)

Comment on lines 87 to 107
md"""
Limb darkening is the natural phenomenon where a star's surface brightness appears to fall off as we look from its center to off-angle towards its limbs. The rate at which it falls off is described by a given limb darkening law. For this example, we are using a polynomial limb darkening law from [Agol, Luger, Foreman-Mackey (2020)](https://ui.adsabs.harvard.edu/abs/2020AJ....159..123A/abstract). This law uses analytically derived integrals, which makes it very fast and numerically accurate. There are other limb darkening laws which share a similar `AbstractLimbDark` interface. This interface lets us create composite laws like `IntegratedLimbDark` or `SecondaryLimbDark` with almost no effort. We will explore these laws and more in further notebooks.

Pulling up the documentation for our particular law, `PolynomialLimbDark`, we see that it just accepts a single parameter `u`, the vector of limb darkening coefficients. These are defined such that:

```math
I(\mu) \propto -\sum_{i=0}^N u_i(1 - \mu)^i, \quad u_0 \equiv -1\quad,
```

where ``μ`` is a dimensionless parameter that varies from ``1`` to ``0`` as we move from the center to the edge of the star, as seen projected on the sky, ``N`` is the order of the polynomial, ``I`` is the intensity of the star, and ``u_i`` are the components of `u`.

For this example, we will use a quadratic limb darkening law ``(N = 2)``:

```math
\begin{align}
I(\mu) &\propto -u_0(1 - \mu)^0 - u_1(1 - μ) - u_2(1 - μ)^2 \\
&= 1 - u_1(1 - μ) - u_2(1 - μ)^2 \quad.
\end{align}
```

"""
Copy link
Member

Choose a reason for hiding this comment

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

This all starts to get a little in the weeds and feels out of place. The "textbook" knowledge is more well-suited for docs like the introduction section.

Rather than writing out the math and spelling out the technical explanation of the coefficients, why not just show a plot comparing quadratic to cubic to N=10 order limb darkening curves. (you don't have to explain how you make this plot; just show it). I think this might give more practical knowledge, which is the goal

Copy link
Member Author

Choose a reason for hiding this comment

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

So, this change was made to address your comment:

Would like to see more explaining what u is- "these are the coefficients used for the higher order (>0) terms of our limb-darkening law" -> "you'll notice the law we declare has one more term than we gave it; that is because there is always a constant term defined to be -1". some stuff like that.

It's not entirely clear to me where you would like to draw the line between practical application and going into the weeds. I think it might be more productive if you just added the specific section you would like to see here when you have the time?

Copy link
Member

Choose a reason for hiding this comment

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

I guess the line I am trying to draw is this is practical knowledge, not as much theoretical knowledge. So I think the goal of the tutorial should be "what is a PolynomialLimbDark, how do I make one, what are the parameters, how does it look". So, e.g. you point out there's an extra term in the struct because that might surprise someone when they're actually working with the package, but I don't think we need to take the extra step and show why the derivation causes that, if that makes sense.

So I still think we can cut out the latex for formula and stick to the description that describes it as a law "describing how the surface brightness changes from the center to the limb" and pair that with a little plot comparing orders and I think that's a good line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, the notebook should be updated now

@mileslucas
Copy link
Member

okay I really like where it's at now! How should we go about incorporating it into the docs? I noticed you've set up the installs locally, is there a setup with binder or something which will work?

@icweaver
Copy link
Member Author

icweaver commented Feb 23, 2021

Cool =] I can start taking a look into binder later this week. I know there's been a lot of discussion about it over on zulip, and Fons and the rest of the Pluto devs have a really neat implementation on the new Computational Thinking site that we might want to emulate.

Saving this convo from slack:
Roger Luo 5 days ago
so I just realize the computational thinking course is doing the exact the same feature I want - I’m wondering how do people implement this here? is it done in the same way as @jelmar Gerritsen described? https://computationalthinking.mit.edu/Spring21/hw0/

Jerry Ling 5 days ago
that notebook is a iFrame right? so it's not what you're doing, they are just brute forcing it by embedding the entire pluto html into that web page (edited)

Fons van der Plas 4 days ago
eventually OfflineHTMLExport will be replaced by the new method used on the mit site (which will be built into pluto)

Fons van der Plas 4 days ago
the code is here:

Fons van der Plas 4 days ago
https://github.com/fonsp/PlutoUtils.jl/blob/static-export/src/Export.jl (edited)

Fons van der Plas 4 days ago
(make sure to be on that branch)

@mileslucas
Copy link
Member

Something you'll want to consider is the static vs. dynamic output, too. For example, currently the transit curve in the bottom of your notebook is flat showing rprs=0. No problem for interactive use, but having it be >0 at default will fix the static version to show the curve.

@icweaver
Copy link
Member Author

Right, we can do that in the meantime. Saving the notebook with some of the curves showing first should also accomplish this, albeit with the scrubber not reflecting the current state. I thought I already did this in the last commit, is it not showing up for you in the static preview?

@mileslucas
Copy link
Member

is it not showing up for you

Ah it was not previously, but now it is, thanks!
Screen Shot 2021-02-25 at 4 44 54 PM

Base automatically changed from master to main March 8, 2021 06:52
@icweaver
Copy link
Member Author

icweaver commented Apr 7, 2021

Apparently html previews with interactive sliders are going to be discussed at PlutoCon. I would definitely be interested in seeing if we can apply it to our notebooks here!

Adding relevant link here: https://github.com/JuliaPluto/PlutoSliderServer.jl

@icweaver icweaver closed this Oct 16, 2021
@icweaver icweaver deleted the learning_notebooks branch October 16, 2021 21:05
@icweaver icweaver mentioned this pull request Oct 16, 2021
2 tasks
@icweaver
Copy link
Member Author

Switched branch name to #28

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