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

Figure 1 #6

Closed
iangrooms opened this issue Jan 14, 2021 · 12 comments
Closed

Figure 1 #6

iangrooms opened this issue Jan 14, 2021 · 12 comments

Comments

@iangrooms
Copy link
Member

I've added a jupyter notebook to the repo with code to make Figure 1. I don't usually use Python to make figures for publication, so I'd be grateful if someone could put this into a 3x2 panel figure with nice labels and consistent line coloring, etc. I'm hoping this is a small job for someone who is good at it, but if not let me know and I'll try to figure it out myself.

@jbusecke
Copy link
Collaborator

I can definitely do that later today. Could you note down the exact titles, axes labels, legend entries you would like on those?

@jbusecke
Copy link
Collaborator

Actually just had a few minutes downtime and took a swing at it in #10. Please let me know what you think @iangrooms

@iangrooms
Copy link
Member Author

After Julius' work it's nearly done. I just need to add a legend and labels, then I'll close this issue.

@jbusecke
Copy link
Collaborator

Glad I could help 😁

@NoraLoose
Copy link
Member

@iangrooms, I am confused about the filter kernels that you constructed for Figure 1. According to our overleaf notes on spatial filters, I would have chosen the following forms for the target filters in Fourier space:

  • Gaussian: FG = lambda z: np.exp(-(32/3)*(z**2)) rather than FG = lambda z: np.exp(-(8/3)*(z**2))
  • Taper: FT = interpolate.PchipInterpolator(np.array([0,1/8,np.pi/8,np.pi]),np.array([1,1,0,0])) rather than FT = interpolate.PchipInterpolator(np.array([0,1/4,np.pi/4,np.pi]),np.array([1,1,0,0]))

Maybe I'm missing something? Especially for the Taper filter... Looking at @jakesteinberg's figure in #1, it seems that Jake does the same thing as Ian's notebook for the taper filter: L_{fs} = L_{fb}/2. For the Gaussian filter, Jake seems to be doing what I suggest above: L_{gs} = L_{fb}/sqrt(6). Let's make sure we are all using the same length scales (and that we do a good job in communicating the right thing to the user).

@iangrooms
Copy link
Member Author

I think the problem is with the mismatch between how 'filter scale' is defined for the Gaussian. In the overleaf notes the factor of sqrt{6} comes from the way filterSpec defines the length scale of the Gaussian, which is not the standard deviation.

There might also be confusion about what size boxcar filter reduces the length scale of the data by a factor of 8. The obvious way is that a boxcar with width 8 reduces the scale by a factor of 8; we had also previously discussed the idea that a boxcar of radius 8 reduces the scale by a factor of 8. The latter is because a boxcar of radius 8 zeros out what would be the Nyquist mode on a grid that is 8 times coarser. I am currently using the former definition.

The taper filter was originally set up so that the first root of the target filter is the Nyquist wavenumber of the "target" grid. I have switched that for the figure so that the boxcar and the taper have the same first root.

I do think the figure is currently self-consistent under the following rules: the boxcar & Gaussian kernels have the same standard deviation (hence the Taylor series for the Fourier transforms match to second order at the origin), and the taper and boxcar have the same first root. We should decide whether these are the "right" rules though, especially since they're a little different than what we have used in the past.

@NoraLoose
Copy link
Member

Re Gaussian filter: You are correct, this was my mistake (I made a simple miscalculation).

There might also be confusion about what size boxcar filter reduces the length scale of the data by a factor of 8. The obvious way is that a boxcar with width 8 reduces the scale by a factor of 8; we had also previously discussed the idea that a boxcar of radius 8 reduces the scale by a factor of 8. The latter is because a boxcar of radius 8 zeros out what would be the Nyquist mode on a grid that is 8 times coarser. I am currently using the former definition.

Re taper filter: Ok, this makes sense now. I think the original notes (e.g., Figure 2 in the filter notes) follow the latter definition, and this is what I have been doing in the past.

I do think the figure is currently self-consistent under the following rules: the boxcar & Gaussian kernels have the same standard deviation (hence the Taylor series for the Fourier transforms match to second order at the origin), and the taper and boxcar have the same first root. We should decide whether these are the "right" rules though, especially since they're a little different than what we have used in the past.

These rules seem reasonable to me! I will use these for comparing Gaussian & sharp filter for the POP data (unless/until we reconsider these rules).

@iangrooms
Copy link
Member Author

I just updated Figure 1 by adding labels and a legend. I have three requests:

  1. Can you make the font size bigger for everything?
  2. In the bottom right panel there is no "target" filter kernel, so the line colors don't match the other panels. It should be (first line)=orange, (second line)=green, (third line)=red. Can you update this?
  3. It would be nice to have a single legend for the whole figure, since the legend for panel [0,0] applies to all the panels

@jbusecke
Copy link
Collaborator

Was this a request to me, @iangrooms ? Happy to try to squeeze that in towards the evening.

@iangrooms
Copy link
Member Author

Sorry - should have been more explicit. I'd be grateful if you can do those @jbusecke, otherwise let me know and I'll try to figure it out.

@jbusecke
Copy link
Collaborator

No problem! I was just parsing through a lot of messages in my inbox. I ll take a look later.

@jbusecke
Copy link
Collaborator

I think I covered all points in #15. Let me know what you think.

@jbusecke jbusecke removed their assignment Jan 21, 2021
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

No branches or pull requests

3 participants