lincc-frameworks / tdastro

MIT License
4 stars 0 forks source link

Fixes for various issues trying to set up first time #91

Closed mtauraso closed 2 months ago

mtauraso commented 2 months ago
review-notebook-app[bot] commented 2 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

github-actions[bot] commented 2 months ago
Before [9fcdf6dd] After [0441ab9e] Ratio Benchmark (Parameter)
6.92±0.05ms 6.86±0.03ms 0.99 benchmarks.time_chained_evaluate

Click here to view all benchmarks.

mtauraso commented 2 months ago

Looks good, thanks for adding this.

Let's discuss docstring style next Tues--no strong personal opinions, and I do really like how explicit the units parameter is here, but would be nice to codify something before we diverge too much.

I was a bit worried when I added that part... Not sure if it really even needs to be there, because I don't know much about what sorts of passband files tdastro needs to support.

I considered astropy.units for this part, but it seemed like too much complexity just to get the piece of code working again after I broke it by introducing the LSST passband file from github for seemingly the first time.

mtauraso commented 2 months ago

Planning to Merge sometime Tuesday (9/3) unless someone asks me not to.