-
Notifications
You must be signed in to change notification settings - Fork 7
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
JOSS Review #143
Comments
Awesome! Thanks for reviewing the paper/package. I am really happy to get some more eyes on the package. I'll start addressing these comments as they come in. |
I got through this tutorial just fine, this time. I really like your approach for the composite models and the hierarchy of primitive types. I'm curious here why not use a product of 2, 3, 5, or 7 (e.g. Primitive Geometric ModelsJust wanted to point out if you try and copy-paste these tutorials, they will fail due to the name clash between DocumentationThere's a lot of docstrings that are slightly mangled, e.g. 1, 2, 3, 4, etc. I think a quick pass through all the docstrings and tidying them up so there's a clear function signature formatted appropriately. You might look for referenced functions, too, for example here should reference |
The repository/docs are missing community guidelines
Adding something to the README and the docs homepage about where to file issues, any standards (e.g. code style, ColPrac), etc is all you need. |
Paper
AGN acronym not introduced (could be introduced in L7)
"disks, rings, and crescents"
"flexible model interface
"just-ahead-of-time compiler Overall great writing- you clearly listed the motivation for Comrade.jl and discussed all the challenges with VLBI imaging. Perhaps some more discussion about the state of the field could be warranted, either in the paper or in the docs. For example, what is the maturity of modeling with Comrade.jl vs eht-imaging? |
PerformanceAre there any benchmarks? You mention in the README the want for speed and proper SIMD, GPU, and distributed support. What is the current status of those goals? |
Thank you for all your comments! Time for me to get to work.
I edited the first part of the introduction to make it a little clearer. Loading Data into ComradeI have fixed the broken link.
This should be fixed now on master. Thanks for catching that. Making an Image of a Black Hole
I like this a lot! I am used to putting my
I completely agree and I have struggled with this myself. Originally I hadn't interfaced with any sampling packages and just made some primitives that would make it easier to interface with samplers, i.e. auto parameter transforms to R^n or the unit hypercube. However, I found that this was asking a lot for my expected user base where a lot of them are coming from python so having something more integrated was useful. I had originally used Requires.jl but that isn't great since I couldn't put version bounds in, so then I settled on my current strategy. In the future, I may move back to my original plan.
Fixed! Modeling with non-analytic Fourier transformsThis all came from an old version of m = ExtendedRing(8.0)
# change diameter
stretched(m, 10.0, 10.0) to fit more in line with the general philosophy of the package. With this change, the I also fixed the modelimage part of the tutorial. This part of Comrade has changed quite a bit recently.
This is a good idea! Fixed in #147 Primitive Geometric ModelsI renamed all the structs to DocstringsYes. I was pretty sloppy here. I'll go through all of them and start fixing them in #148 Community GuidelinesI've added this to the README and a brief statements in the docs. PaperI'll update the typos ASAP. A comparison to eht-imaging is interesting since it partially uses pretty different methods. eht-imaging uses regularized maximum likelihood (RML) imaging methods to produce images with an objective function where Benchmarks
That part of the README was pretty old so I removed it. GPU support is something planned but it will require a bit of work so I don't want to promise anything yet. I can try to put some benchmarks compared to eht-imaging modeling pacakge later this week. I have another larger and flashier benchmark to an EHT package called Themis, but it is difficult since
|
@mileslucas alright here are some preliminary benchmarks. The benchmark evaluates the model I presented in the paper using both Comrade and eht-imaging Comrade julia> @benchmark visibilities($m, $u, $v)
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
Range (min … max): 91.464 μs … 584.311 μs ┊ GC (min … max): 0.00% … 0.00%
Time (median): 98.503 μs ┊ GC (median): 0.00%
Time (mean ± σ): 108.179 μs ± 28.756 μs ┊ GC (mean ± σ): 0.00% ± 0.00%
█▂▁▇█▂ ▂▅ ▃ ▁ ▁▁▁▁▁▁▁ ▁▂ ▂
█████████▇▇▆██▅▆▄▇█▄▄▂▃▃█▄▂▂▃▃▃▄▆▄▄▂▄▃▄▃▃▄▆▆▆▇▇██████████████ █
91.5 μs Histogram: log(frequency) by time 198 μs <
Memory estimate: 4.50 KiB, allocs estimate: 2. eht-imaging julia> @benchmark meh.sample_uv($u, $v)
BenchmarkTools.Trial: 4445 samples with 1 evaluation.
Range (min … max): 898.894 μs … 5.173 ms ┊ GC (min … max): 0.00% … 0.00%
Time (median): 1.034 ms ┊ GC (median): 0.00%
Time (mean ± σ): 1.118 ms ± 240.445 μs ┊ GC (mean ± σ): 0.00% ± 0.00%
▂▄▁▁█▇▄▃▇▆▂▁▁▃▁ ▁▁▁▁▁▁ ▁
▅▃████████████████▆▆▇▆▇▅▃▆▅▆▁▁▄▄▄▄▃▃▅▄▁▃▃▄▁▃▁▅▄▃▅▇▆▇▇▇▇██████ █
899 μs Histogram: log(frequency) by time 1.98 ms <
Memory estimate: 6.22 KiB, allocs estimate: 39. So that is an order of magnitude speed up! I am actually a bit surprised that it is so much since the major computational cost should just be bessel functions, but hey I guess compiling does matter! |
Here are some more benchmarks. I looked at the 2017 M87 EHT data and looked at likelihood evaluation and gradient times:
This is actually really really good. I am specifically pleased with the Themis comparison since it is written in C++. This is probably because of the different bessel function implementation since Themis uses a handwritten one, while Comrade uses the one from SpecialFunctions.jl. This is actually by design since Themis was written to have as few dependencies as possible so most of the code was rewritten from scratch (I wrote a bunch of it and tbh it isn't great). On the other hand Comrade tries to use the Julia ecosystem as much as possible. The gradient time are also a little surprising since eht-imaging uses handwritten gradients for everything so that fact that ForwardDiff.jl is doing so well is encouraging. For Themis it's poor benchmark was expected since it doesn't compute gradients using autodiff or handwritten but instead relies on a finite difference scheme which is pretty damn slow. |
@ptiede those are some nice benchmark results! I think you could just paste those outputs into your docs (maybe Home/Benchmarks or maybe a separate page) with a snippet of some of your Example system information
With the contribution guidelines checked off, you've got all the pieces done for the JOSS paper. Once you merge in those PRs I'll close out my review with JOSS. |
move to next prod based on JOSS Review #143
I added the benchmarks to the docs and included some code to reproduce the results for eht-imaging and Comrade. |
Awesome, it's looking great! Thanks for contributing to the Julia astronomy ecosystem, I'm always happy to see the community grow :) cheers |
necro-linking the JOSS issue openjournals/joss-reviews#4457 |
Hey @ptiede, happy to be reviewing this JOSS submission. I'll be using this issue for working discussions and problems I find!
I'll start with doc typos I've found; this is what I've caught while reading through-
this sentence is missing a verb, I think?
Comrade.jl/docs/src/vlbi_imaging_problem.md
Line 3 in 79b66f8
Loading Data into Comrade
dead link, should point to
achael/eht-imaging
Comrade.jl/docs/src/examples/data.jl
Line 17 in 79b66f8
During this part of the first tutorial
I got some warnings
that seem like they can be addressed.
Making Image of a Black Hole
Very minor comment: for this tutorial, I think it would be clearer if you put the package imports where they are used- for one, if the user isn't interested in, say, the AHMC part of the tutorial, then they won't need to install/load the package. Second, it makes it super obvious that "I need this to enable these things", like you don't need
Distributions.jl
until you create the priors, and you don't needGalacticOptim.jl
andComradeGalactic.jl
until you find the MAP estimate.(Tangential note: There's something flagrantly un-Julian about having to make 5 unique packages for 5 unique optimization algorithms when the language is build on composability. I don't have a solution, and this is fairly commonplace, but it's obviously a lot of extra work for you as a maintainer and is annoying at minimum as a user)
Problems:
not clear what "# from EHTC VI 2019" is supposed to mean, and extraneous period.
Modeling with non-analytic Fourier transforms
seems to be invalid syntax, should be
seems like this part of the tutorial is out of date.
Taking a break for now, will keep adding as I continue reviewing the code!
The text was updated successfully, but these errors were encountered: