Add glass map #164

BrianGun · 2021-04-30T02:02:20Z

Pull Request Template

Description

Added a function to draw glass maps of glass catalogs

Fixes #163

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Visual verification of plot

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
I have checked my code and corrected any misspellings

Fixes #163 got basic scatter plot done but there are weird values in the NIKON glass catalog that are messing things up.

…glasses that have nearly identical [index,dispersion] points so the names don't overlap. Still need to figure out how to get markers not to draw in scatter plot. Setting markershape = :none doesn't work.

friggog · 2021-04-30T06:44:48Z

Maybe marker=:none or marker=nothing? I remember plots args being a real pain and often just not working at all...

src/GlassCat/utilities.jl

Fixes #163 got basic scatter plot done but there are weird values in the NIKON glass catalog that are messing things up.

Could be improved to show popup menu of glasses that have nearly identical [index,dispersion] points so the names don't overlap. Still need to figure out how to get markers not to draw in scatter plot. Setting markershape = :none doesn't work.

* whitespace * export glasscatmap * format docstring * typed function signature * u"nm" -> nm * chained comparisons and simplified boolean expression

BrianGun · 2021-04-30T17:28:53Z

I tried marker = :none and that didn't do anything. The markers still showed up so instead I scaled them to size 0. Which only made them small, not invisible.

Maybe marker=:none or marker=nothing? I remember plots args being a real pain and often just not working at all...

Fixes #163 merging with new changes on main branch

…s an old name for the function.

Fixes #163 added title to glass catalog map

BrianGun · 2021-05-01T00:10:49Z

@friggog or @alfredclwong could you review and approve if there are no remaining issues?

Are there any remaining questions? In the future we can change the behavior with respect to glasses that have _ in the name. Are there any blocking issues?

set the alpha of the markers to zero so now they don't draw. It's a hack but seems to work.

Fixes #163 set markeraplha = 0 so the marker shapes don't show up in the glassmap plot.

alfredclwong · 2021-05-04T08:18:45Z

src/GlassCat/utilities.jl

+            g = x -> ForwardDiff.derivative(f, x);
+            dispersion = g(wavelength)
+
+            if (mindispersion <= dispersion <= maxdispersion) && (showprefixglasses ? true : !hasprefix)    #don't show glasses that have an _ in the name. This prevents cluttering the map with many glasses of similar (index,dispersion).  


I have a strong preference for the regular boolean expression (showprefixglasses || !hasprefix) as opposed to the ternary expression (showprefixglasses ? true : !hasprefix). Was there a reason for adding this back in?

alfredclwong · 2021-05-04T08:20:53Z

src/GlassCat/utilities.jl

+        end
+    end
+    series_annotations = Plots.series_annotations(glassnames, Plots.font(family = "Sans", pointsize = glassfontsize, color = RGB(0.0,0.0,.4)))
+   scatter(dispersions,indices, xaxis = "dispersion", yaxis = "index", series_annotations = series_annotations, markersize = .001, legends = :none, markeralpha = 0.0, markershape = :none, title = "Glass Catalog: $glasscatalog")


Are markersize and markershape still needed?

alfredclwong · 2021-05-05T16:25:03Z

@friggog or @alfredclwong could you review and approve if there are no remaining issues?

Are there any remaining questions? In the future we can change the behavior with respect to glasses that have _ in the name. Are there any blocking issues?

set the alpha of the markers to zero so now they don't draw. It's a hack but seems to work.

@BrianGun I posted a few questions above. I think it's safe to remove markersize and markershape, but it might be worth running the code again to see if it still works the way you want it to.

Fixes #163 added comment explaining markeraplha = 0 argument to scatter.

BrianGun · 2021-05-05T21:08:10Z

@alfredclwong everything looks fine. However I cannot verify that the examples are rendering correctly since there is some obscure problem on my local machine that prevents any of the example images from being rendered. Could you verify the glassmap example shows up properly in the Examples docs and then approve?

alfredclwong · 2021-05-05T23:50:34Z

@alfredclwong everything looks fine. However I cannot verify that the examples are rendering correctly since there is some obscure problem on my local machine that prevents any of the example images from being rendered. Could you verify the glassmap example shows up properly in the Examples docs and then approve?

I don't see any changes to the Examples docs on this branch. Are the changes pushed? If you're having problems with the docs locally, you should still be able to see them here https://microsoft.github.io/OpticSim.jl/previews/PR164/examples/.

BrianGun · 2021-05-06T20:18:00Z

@alfredclwong The changes aren't in the Examples docs. They are in the GlassCat function docs. The new function shows up there in the preview docs. Everything seems to be working. Could you approve?

alfredclwong · 2021-05-06T20:23:23Z

@alfredclwong The changes aren't in the Examples docs. They are in the GlassCat function docs. The new function shows up there in the preview docs. Everything seems to be working. Could you approve?

Ah I thought you meant that you'd put in an example of the plot somewhere. Looks good.

BrianGun added 2 commits April 29, 2021 17:18

Draw glass map

eb683ec

Fixes #163 got basic scatter plot done but there are weird values in the NIKON glass catalog that are messing things up.

glassmap drawing is working. Could be improved to show popup menu of …

4288414

…glasses that have nearly identical [index,dispersion] points so the names don't overlap. Still need to figure out how to get markers not to draw in scatter plot. Setting markershape = :none doesn't work.

This comment has been minimized.

Sign in to view

friggog changed the title ~~BrianGun/issue163~~ Add glass map Apr 30, 2021

alfredclwong reviewed Apr 30, 2021

View reviewed changes

src/GlassCat/utilities.jl Outdated Show resolved Hide resolved

src/GlassCat/utilities.jl Outdated Show resolved Hide resolved

BrianGun and others added 3 commits April 30, 2021 15:31

Draw glass map

26ff724

Fixes #163 got basic scatter plot done but there are weird values in the NIKON glass catalog that are messing things up.

glassmap drawing is working

b7f2e31

Could be improved to show popup menu of glasses that have nearly identical [index,dispersion] points so the names don't overlap. Still need to figure out how to get markers not to draw in scatter plot. Setting markershape = :none doesn't work.

clean up

a8db36c

* whitespace * export glasscatmap * format docstring * typed function signature * u"nm" -> nm * chained comparisons and simplified boolean expression

alfredclwong force-pushed the BrianGun/issue163 branch from 30e50ae to a8db36c Compare April 30, 2021 14:32

BrianGun added 7 commits April 30, 2021 11:59

Draw glass map

8ab6feb

Fixes #163 merging with new changes on main branch

changed export to export drawglassmap instead of glasscatmap, which i…

97b0e83

…s an old name for the function.

Draw glass map

6f756e3

Fixes #163 added title to glass catalog map

mode to documentation for drawglassmap

c23c634

changed sign on dispersion to make it mathematically correct.

f52af58

added bounds on min and max dispersion, shortened argument names

f28d9a2

changed title of plot

cda6f9f

BrianGun and others added 2 commits April 30, 2021 17:17

Draw glass map

4f9ae66

Fixes #163 set markeraplha = 0 so the marker shapes don't show up in the glassmap plot.

Merge branch 'main' into BrianGun/issue163

5ddaea4

alfredclwong force-pushed the BrianGun/issue163 branch from 21b0b30 to 88adb96 Compare May 4, 2021 08:22

alfredclwong reviewed May 4, 2021

View reviewed changes

alfredclwong added 3 commits May 4, 2021 09:54

add drawglassmap to docs

4278779

formatting

79d0465

remove ternary expression and unecessary(?) scatter args

0a80b79

alfredclwong force-pushed the BrianGun/issue163 branch from 88adb96 to 0a80b79 Compare May 4, 2021 08:54

escape underscores in docstring

2d3dec9

alfredclwong force-pushed the BrianGun/issue163 branch from 8931e82 to 2d3dec9 Compare May 4, 2021 10:46

Merge branch 'main' into BrianGun/issue163

6ddcb14

BrianGun and others added 2 commits May 5, 2021 09:37

Merge branch 'main' into BrianGun/issue163

34b01f2

Draw glass map

7b35f82

Fixes #163 added comment explaining markeraplha = 0 argument to scatter.

alfredclwong approved these changes May 6, 2021

View reviewed changes

BrianGun merged commit 69b8b74 into main May 6, 2021

BrianGun deleted the BrianGun/issue163 branch May 6, 2021 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add glass map #164

Add glass map #164

BrianGun commented Apr 30, 2021 •

edited by friggog

Loading

This comment has been minimized.

friggog commented Apr 30, 2021 •

edited

Loading

BrianGun commented Apr 30, 2021 •

edited

Loading

BrianGun commented May 1, 2021 •

edited

Loading

alfredclwong May 4, 2021

alfredclwong May 4, 2021

BrianGun May 5, 2021

alfredclwong commented May 5, 2021

BrianGun commented May 5, 2021 •

edited

Loading

alfredclwong commented May 5, 2021

BrianGun commented May 6, 2021

alfredclwong commented May 6, 2021

Add glass map #164

Add glass map #164

Conversation

BrianGun commented Apr 30, 2021 • edited by friggog Loading

Pull Request Template

Description

Type of change

How Has This Been Tested?

Checklist:

This comment has been minimized.

friggog commented Apr 30, 2021 • edited Loading

BrianGun commented Apr 30, 2021 • edited Loading

BrianGun commented May 1, 2021 • edited Loading

alfredclwong May 4, 2021

Choose a reason for hiding this comment

alfredclwong May 4, 2021

Choose a reason for hiding this comment

BrianGun May 5, 2021

Choose a reason for hiding this comment

alfredclwong commented May 5, 2021

BrianGun commented May 5, 2021 • edited Loading

alfredclwong commented May 5, 2021

BrianGun commented May 6, 2021

alfredclwong commented May 6, 2021

BrianGun commented Apr 30, 2021 •

edited by friggog

Loading

friggog commented Apr 30, 2021 •

edited

Loading

BrianGun commented Apr 30, 2021 •

edited

Loading

BrianGun commented May 1, 2021 •

edited

Loading

BrianGun commented May 5, 2021 •

edited

Loading