ilikecubesnstuff / commensurability

Packages for calculating orbit commensurabilities.
https://commensurability.readthedocs.io/en/latest/
MIT License
2 stars 2 forks source link

📝: Comments Tracker #17

Open nstarman opened 2 months ago

nstarman commented 2 months ago

A well-written paper! I have some suggestions from my first read through. I'm happy to discuss any points.

  1. https://github.com/ilikecubesnstuff/commensurability/blob/ce0ab67a5ca17524eafd9c4fbfd5e9a4624a350e/paper/paper.md?plain=1#L31

In the 2nd sentence I suggest changing "the Sun" to "a star" because the scope of the first sentence is all stars similar to the sun, not just the sun, and the second sentence applies to all these stars, not just the sun.

  1. "However, a special class of stars are on orbits"

I don't think this is quite what you mean. The orbit of a star is an extrinsic, not intrinsic property. It's not that the star is in a special class, it is its orbit. How about something like: "However there are special classes of stellar orbits" ?

  1. "around the galactic center"

Capitalize "galactic"?

  1. "These orbits are called “commensurate”. "

The wording here is a bit confusing to me. Just to work through my own thought process, for resonant orbits I would say that something is on a resonant orbit, or that any orbit with X set of properties is resonant. Within that I would say that there are many families of resonant orbits, and that two orbits are "the same" (commensurate) if they are in the same family. Looking at https://academic.oup.com/mnras/article/500/1/838/5925365, "commensurate" is first introduced in this way, as indicating two orbits belong in the same orbital family, but then immediately after it is also taken to mean the whole class of orbits that which doesn't fill its toroid, not just the relationship between orbits in an orbital family. For me the sentence "These orbits are called “commensurate”. " is then confusing because "commensurate" holds this dual definition and so I look to the word "These" to try to contextualize. However "These" is vague and isn't clarifying. In sum, from 10.1093/mnras/staa3202 nothing seems incorrect, per se, just confusing.

  1. "astronomers will often use"

I recommend the more active: "astronomers ~will~ often use"

  1. "models for galaxies that allow for the integration of orbits within the model potential"

Maybe "models for galaxies that allow for the integration of orbits within the model galaxy's potential", just to be a tad more explicit.

  1. "time series of positions (e.g. $[x,y,z]$)"

How about "$x,y,z$" to show it's a time series of positions?

  1. "Complicating... a new frequency: the pattern frequency of the evolution."

I suggest working the word "frequency" into the previous sentence. I get what you mean, but it's not a "new" frequency if we haven't had any other frequencies discussed.

  1. "The evolution of the potential might be"

Sorry of the pedantry. I think a word is missing here. The causes are imprecise. The evolution is not the rotation, but caused by it. How about: "The evolution of the potential might be [sourced / caused by]"

  1. "the interaction of"

Maybe "the interaction with"?

  1. "Orbit tessellation aims to pick out commensurate orbits with relatively short orbit integration times"

I think there's a few small issues with this sentence.

In total, I'm suggesting something like "The method of orbit tessellation aims to pick out commensurate orbits and requires only relatively short orbit integration times to do so" (I think there's a better sentence than what I've written. This is for illustration purposes only).

  1. "The Python package presented here, commensurability"

Should the package be typeset differently, e.g. a commensurability ?

  1. "The commensurability package solves both problems while providing interoperability with three leading galactic dynamics packages."

The "while" doesn't feel like the right conjuction. I think "and" is more appropriate, in which case this would read "The commensurability package solves both problems and provides interoperability with three leading galactic dynamics packages [to accomplish the underlying orbit integrations]."

  1. "While frequency analysis is a useful tool, ambiguity in the classification remains."

Maybe explain more in this sentence what "ambiguity" remains? Also the Yoda sentence structure feels a bit funny here. E.g. "While frequency analysis is a useful tool, there remains ambiguity in the classification of [X type orbits from Y type orbits.]."

  1. "Orbit tessellation avoids these pitfalls."

Maybe add a clause explaining how.

  1. "search the model potential space for unique orbits."

Correct me if I'm wrong, but it's not that the orbits are unique, it's that the orbital families are distinct? So maybe "search the model potential space for distinct orbital families".

  1. "Although TessellationAnalysis objects focus on orbit tessellation specifically, the analysis framework can be easily extended to any orbit evaluation method."

Maybe sharpen what you mean a little? The base class of TessellationAnalysis is easily extended to other methods, not tessellation base? It's not clear to me how TessellationAnalysis can be extended to non-tessellations.

  1. "This subpackage can be used for its orbit evaluation function independent of commensurability."

Sounds useful! Not for this paper, but perhaps in future consider spinning this off as a separate package?

— openjournals/joss-reviews#7009