Closed editorialbot closed 1 year ago
editorialbot
commented
2 years ago Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.
For a list of things I can do to help you, just type:
@editorialbot commandsFor example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdfSoftware report:
github.com/AlDanial/cloc v 1.88 T=0.25 s (921.5 files/s, 115608.3 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 39 4200 5995 12733
reStructuredText 173 631 898 697
Jupyter Notebook 6 0 2821 227
TeX 1 8 0 114
Markdown 2 11 0 31
YAML 2 7 18 30
DOS Batch 2 8 1 27
make 1 4 7 9
TOML 1 0 0 3
-------------------------------------------------------------------------------
SUM: 227 4869 9740 13871
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repositoryWordcount for paper.md is 647
editorialbot
commented
2 years ago Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1088/978-0-7503-1320-9 is OK
- 10.1088/0067-0049/216/2/29 is OK
- 10.1093/mnras/stv1848 is OK
- 10.1093/mnras/stv817 is OK
- 10.1093/mnras/staa1915 is OK
- 10.1111/j.1365-2966.2011.19412.x is OK
MISSING DOIs
- None
INVALID DOIs
- None:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
christinahedges
commented
2 years ago @webbjj, @lwang-astro, @rieder – This is the review thread for the clustertools paper. Please don't hesitate to message me here if you have questions (use @christinahedges). ✉️
Please read the "Reviewer instructions & questions" in the first comment above to get started. If you get lost, you can also see the reviewer guidelines. You will have to generate your review "checklist" by adding the comment @editorialbot generate my checklist to this thread.
To review for JOSS ,@lwang-astro and @rieder will step through that checklist for clustertools and assess the package. As you go over the submission, please check any items that you feel have been satisfied. If you are concerned about a requirement, please discuss it here on this thread 🧵 . Feel free to post about questions/concerns as they come up as you go through your review. Discussion between the authors/reviewers and myself is encouraged!
The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention this issue (https://github.com/openjournals/joss-reviews/issues/4483) so that a link is created to this thread (and I can keep an eye on what is happening).
We aim for the review process to be completed within about 4-6 weeks but please make a start well ahead of this as JOSS reviews are by their nature iterative and any early feedback you may be able to provide to the author will be very helpful in meeting this schedule. When you're finished with your checklist, leave a comment and @ me to let everyone know your review is complete.
christinahedges
commented
2 years ago FYI for both reviewers: In the pre-review we discussed with the submitting authors that this tool is designed to improve the interface with products from several other tools. We believe this type of tool can still count under JOSS's requirement for substantial scholarly effort and can meet the JOSS requirements including having an obvious research application and utility to the community. I want to bring your attention to this during your review, to make sure that I can check in with both reviewers that they agree this tool meets those standards.
rieder
commented
2 years ago rieder
commented
2 years ago A question (@webbjj): The review specifies version 1.0 of Clustertools. However, it seems that this version was released two years ago (according to the GitHub tag). I guess we should look at the current version instead? Maybe a new tag would be appropriate?
lwang-astro
commented
2 years ago 
webbjj
commented
2 years ago Thanks for the suggestion @rieder . I don't have much experience with updating tags/releases so wasn't aware of this option. I just renamed the previous tag, which dates back two years, to 0.1. This had been the version number until I submitted to JOSS. The current version and release have now been tagged as 1.0. Hope that clears things up.
christinahedges
commented
2 years ago @lwang-astro and @rieder, this is a gentle check in to see how you're going with your review. It seems like you are both started and working on the review, please reach out if you need anything or have any questions.
lwang-astro
commented
2 years ago One issue related to installation @webbjj When I try the first installation method
git clone https://github.com/webbjj/clustertools.git
cd clustertools
python setup.py installI obtain an error
running install
error: can't create or remove files in install directory
The following error occurred while trying to add or remove files in the
installation directory:
[Errno 13] Permission denied: '/usr/local/lib/python2.7/dist-packages/test-easy-install-118055.write-test'
The installation directory you specified (via --install-dir, --prefix, or
the distutils default setting) was:
/usr/local/lib/python2.7/dist-packages/
Perhaps your account does not have write access to this directory? If the
installation directory is a system-owned directory, you may need to sign in
as the administrator or "root" account. If you do not have administrative
access to this machine, you may wish to choose a different installation
directory, preferably one that is listed in your PYTHONPATH environment
variable.
For information on other options, you may wish to consult the
documentation at:
https://setuptools.readthedocs.io/en/latest/easy_install.html
Please make the appropriate changes for your system and try again.@webbjj From document Initialization: To initialize a StarCluster, one can simply start with:
import clustertools
cluster=StarCluster()This is incorrect, without
from clustertools import StarCluster
or it should be
cluster=clustertools.StarCluster()
webbjj
commented
2 years ago Thanks @lwang-astro .
I haven't tested the code with anything less than python 3, so I am wondering if that is the issue with the install error. Just to confirm, you normally have permission to install in the directory '/usr/local/lib/python2.7/dist-packages/test-easy-install-118055.write-test'? If so I will try and reproduce the error and track down the problem. It will be helpful to know if it works with python3, but I understand if you are unable to test that.
And thanks for the heads up on the docs, fixing now.
rieder
commented
2 years ago I would suggest not supporting any Python versions below 3.5 or so. Those older versions are phased out / unsupported anyway, and it avoids a lot of headaches.
lwang-astro
commented
2 years ago @webbjj This installation error is independent of Python vesion, but relate to the default path to install, the same error occur for Python3.8.
[lwang@Thinkpad-x1:~/code/clustertools]$ python3 setup.py install
running install
error: can't create or remove files in install directory
The following error occurred while trying to add or remove files in the
installation directory:
[Errno 13] Permission denied: '/usr/local/lib/python3.8/dist-packages/test-easy-install-231.write-test'
The installation directory you specified (via --install-dir, --prefix, or
the distutils default setting) was:
/usr/local/lib/python3.8/dist-packages/
Perhaps your account does not have write access to this directory? If the
installation directory is a system-owned directory, you may need to sign in
as the administrator or "root" account. If you do not have administrative
access to this machine, you may wish to choose a different installation
directory, preferably one that is listed in your PYTHONPATH environment
variable.
For information on other options, you may wish to consult the
documentation at:
https://setuptools.readthedocs.io/en/latest/easy_install.html
Please make the appropriate changes for your system and try again.For Linux system (Ubuntu), standard users don't have permission to install package in '/usr/local/lib/'
It only works when 'sudo' is used:
sudo python3 setup.py install
I think that this is a general feature for Python installation for either setup.py or pip3, not a problem from clustertools.
For pip3, I can successfully install the package if --user option is provided (pip3 install --user clustertools). Otherwise, the same issue occurs.
Perhaps this issue does not occur when the package is installed via anaconda or python virtual environment.
I suggest to include some sentences in the manual that 'sudo' is needed to avoid permission error or how to manually set the install path.
lwang-astro
commented
2 years ago @webbjj A few questions
1.
Now I have successfully loaded a snapshot of Plummer model with the half-mass radius of 1 pc using add_star.
The snapshot use units of pc, pc/Myr, Msun.
Then I set
cluster.units = "pcmyr"
cluster.origin = "centre"
cluster.analyze(sortstars=True)However, the calculation of virial_radius looks not correct:
In [42]: cluster.virial_radius()
Out[42]: 5.318303126871492e-05rcore function as described in the document:
In [43]: cluster.rcore()
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
Input In [43], in <module>
----> 1 cluster.rcore()AttributeError: 'StarCluster' object has no attribute 'rcore'
@rieder - thanks for the heads up on python 2.7. I looked at it quickly and seems like a headache and not worth it. I image going from 2.7 to 3 was a lot of work for AMUSE.
@lwang-astro
lwang-astro
commented
2 years ago @rieder - thanks for the heads up on python 2.7. I looked at it quickly and seems like a headache and not worth it. I image going from 2.7 to 3 was a lot of work for AMUSE.
@lwang-astro
- I have updated the installation instructions for users that might not have permissions to install in certain directories. I agree it was bad form to assume a user knows how to deal with such issues.
- I have unfortunately not been able to reproduce the issue with either the virial_radius or rcore functions. Did you use pip install or git clone? If the former then perhaps I need to update the pip. The pcmyr have been added recently as I get ready to add PeTar support, so there is a chance there is a bug that my tests have missed as it has not been as heavily used as pckms. Alternatively maybe you could send the input file?
I use the version installed from the pip. Maybe it is not a up-to-date version yet.
webbjj
commented
2 years ago @lwang-astro - I have pushed a new version (1.0.1) to pip. My apologies, as I thought I updated the pip version when I submitted to JOSS.
lwang-astro
commented
2 years ago @webbjj Thank for the update. The new pip version have included the rcore function. But I notice two new issues.
rcore is different from the definition used in NBODY6 and PeTar codes.I used mcluster code (https://github.com/lwang-astro/mcluster) to generate a Plummer model with
rvir = 1.20395 rh = 1.00000 rplummer = 0.76628.
The corresponding commander: mcluster -N 1000 -P 0 -R 1 -C 5 -u 1 -o N1k >mc.log.
Using the input data, the returned value from clustertools:
cluster.rcore()
0.7325688314176657
cluster.virial_radius()
1.202307596486262It seems that the rcore value is close to rplummer (a), which is the scale radius of Plummer potential. This is different from the usually assumed core radius, such as the Casertano & Hut (1985) method with the squares of densities used in NBODY6 (also in PeTar). The corresponding value for this example is approximately 0.4465.
mcluster -N 1000 -P 0 -R 1 -b 1 -C 5 -u 1 -o N1kb >mc.log.).
Then the clustertools return a very small virial radius:
cluster.virial_radius()
4.6897395944288934e-05I think the c.m. velocities of binaries are needed to used to calculate velocity dispersion. Is any way in clustertools to set binary population like add_stars? It seems that the number of binary is only available in the arguments of add_nbody6 function. Is any general way to include binaries?
webbjj
commented
2 years ago Thanks for this check @lwang-astro
The fixes have been pushed to github and pip
Two questions for you @lwang-astro:
lwang-astro
commented
2 years ago @webbjj Now the add_star with binaries work fine.
Your Petar/tools code for the Casertano & Hut (1985) method obviously served as inspiration for what I have done. Is it OK to put the main PeTar paper as a suggested reference for anyone using the rcore command?
Thank you. no problem.
In PeTar/tools where you using the Casertano & Hut (1985) method you find the mass within r6 as nb_mass_tot6=np.sum(particle[nb_index_list6].mass,axis=1) + particle.mass. I am curious if there is a bug here, or perhaps I misunderstanding either KDTree or the method, because to me it looks like the central star mass is being counted twice since nb_index_list6 should include the central star. Furthermore, I think the Casertano & Hut (1985) method ignores the outermost star in the mass calculation. So shouldn't it be np.sum(particle[nb_index_list6].mass,axis=1) - particle.mass[nb_index_list6[:,-1]]. That is how I have implemented it in clustertools and it seems to calculate the expected core radius of the Plummer sphere you generate (albeit with 10K particles instead of 1K particles). Interested in your thoughts.
Thank you for the check. Indeed here is a bug in petar tool. I have a misunderstood of KDTree.query before. The corrected version of petar tool should be:
76 # 6 nearest neighbors
77 nb_r_list6, nb_index_list6 = kdtree.query(particle.pos,k=7)
78 nb_mass_tot6=np.sum(particle.mass[nb_index_list6[:,:-1]],axis=1)
79
80 nb_inv_r6 = 1/nb_r_list6[:,-1]
81 rho = nb_mass_tot6*(nb_inv_r6*nb_inv_r6*nb_inv_r6)
106 rho2 = particle.density*particle.density
107 rc = np.sqrt((particle.r2*rho2).sum()/(rho2.sum()))The new version use k=7 (6 neighbors + itself) Then the summation remove last one as you suggested. The radius need to use the outmost member.
After the correction, I found the core radius from PeTar tools seems to be consistent with mcluster result. I update the pip version of clustertool and found that the cluster.rcore(method='casertano') is 0.52 for a sample of N=1000 from mcluster with no binary. For comparison, petar tool returns 0.4532 and mcluster returns 0.452780. In mcluster and petar tool, the square weighted version is used, which is different from the original Casertano & Hut method. I guess this may cause the difference of results from clustertool and petar tool.
lwang-astro
commented
2 years ago Another question, I use jupyter to run clustertools with plot=True and it does not work
lrprofn, sigvprof= clustertools.sigv_prof(cluster,plot=True)
CalledProcessError Traceback (most recent call last)
File ~/.local/lib/python3.8/site-packages/matplotlib/texmanager.py:233, in TexManager._run_checked_subprocess(self, command, tex, cwd)
232 try:
--> 233 report = subprocess.check_output(
234 command, cwd=cwd if cwd is not None else self.texcache,
235 stderr=subprocess.STDOUT)
236 except FileNotFoundError as exc:
File /usr/lib/python3.8/subprocess.py:415, in check_output(timeout, *popenargs, **kwargs)
413 kwargs['input'] = empty
--> 415 return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
416 **kwargs).stdout
File /usr/lib/python3.8/subprocess.py:516, in run(input, capture_output, timeout, check, *popenargs, **kwargs)
515 if check and retcode:
--> 516 raise CalledProcessError(retcode, process.args,
517 output=stdout, stderr=stderr)
518 return CompletedProcess(process.args, retcode, stdout, stderr)
CalledProcessError: Command '['latex', '-interaction=nonstopmode', '--halt-on-error', '../aba7304bf2416c0194a8a27472595813.tex']' returned non-zero exit status 1.
The above exception was the direct cause of the following exception:
@lwang-astro - interesting. My students and I all use clustertools in jupyter and I have yet to see this error. It might be a version issue, in which case i should add that to the setup file.
Are you able to make other plots in jupyter with latex symbols in the axis labels?
lwang-astro
commented
2 years ago @webbjj I have no problem to use latex format for axe label. such as axe.set_label(r'\sigma') works for me.
webbjj
commented
2 years ago Hmmm, well that is interesting. So on my end I am using:
matplotlib version 3.4.2 jupyter lab version 3.4.3
and I can run everything in clustertools/docs/source/notebooks/profiles.ipynb.
I am not sure where matplotlib looks for the latex plug in. I made sure the latex module was up to date though. I have also found that the galpy.util sometimes messes with my plotting. What version of galpy do you have? I have 1.8.1.dev0.
How does this compare to your setup?
lwang-astro
commented
2 years ago @webbjj I have tried the profile.ipynb and the problem occur again. Here I show the full error message. It suggests that RuntimeError: latex was not able to process the following string: b'lp'
CalledProcessError Traceback (most recent call last)
File ~/.local/lib/python3.8/site-packages/matplotlib/texmanager.py:233, in TexManager._run_checked_subprocess(self, command, tex, cwd)
232 try:
--> 233 report = subprocess.check_output(
234 command, cwd=cwd if cwd is not None else self.texcache,
235 stderr=subprocess.STDOUT)
236 except FileNotFoundError as exc:
File /usr/lib/python3.8/subprocess.py:415, in check_output(timeout, *popenargs, **kwargs)
413 kwargs['input'] = empty
--> 415 return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
416 **kwargs).stdout
File /usr/lib/python3.8/subprocess.py:516, in run(input, capture_output, timeout, check, *popenargs, **kwargs)
515 if check and retcode:
--> 516 raise CalledProcessError(retcode, process.args,
517 output=stdout, stderr=stderr)
518 return CompletedProcess(process.args, retcode, stdout, stderr)
CalledProcessError: Command '['latex', '-interaction=nonstopmode', '--halt-on-error', '../aba7304bf2416c0194a8a27472595813.tex']' returned non-zero exit status 1.
The above exception was the direct cause of the following exception:
RuntimeError Traceback (most recent call last)
Input In [8], in <module>
----> 1 rprof, pprof, nprof=cts.rho_prof(cluster,plot=True)
File ~/.local/lib/python3.8/site-packages/clustertools/analysis/profiles.py:222, in rho_prof(cluster, mmin, mmax, rmin, rmax, nrad, vmin, vmax, emin, emax, kwmin, kwmax, npop, indx, bins, projected, normalize, plot, **kwargs)
219 if normalize:
220 x/=cluster.rm
--> 222 _lplot(
223 x,
224 y,
225 xlabel=xlabel,
226 ylabel=ylabel,
227 title="Time = %f" % cluster.tphys,
228 log=kwargs.pop('log',True),
229 overplot=overplot,
230 filename=filename,
231 **kwargs,
232 )
234 if filename != None:
235 plt.savefig(filename)
File ~/.local/lib/python3.8/site-packages/clustertools/util/plots.py:312, in _lplot(x, y, ltype, xlabel, ylabel, legend, title, xlim, ylim, scale, log, logx, logy, filename, overplot, **kwargs)
247 def _lplot(
248 x,
249 y,
(...)
263 **kwargs
264 ):
265 """ Wrapper for plotting lines with matplotlib.pyplot.plot
266 - allows for most pyplot commands to be assigned when calling the function
267
(...)
310 2018
311 """
--> 312 return _plot(
313 x,
314 y,
315 ptype=ltype,
316 xlabel=xlabel,
317 ylabel=ylabel,
318 legend=legend,
319 title=title,
320 xlim=xlim,
321 ylim=ylim,
322 scale=scale,
323 log=log,
324 logx=logx,
325 logy=logy,
326 filename=filename,
327 overplot=overplot,
328 **kwargs
329 )
File ~/.local/lib/python3.8/site-packages/clustertools/util/plots.py:239, in _plot(x, y, ptype, xlabel, ylabel, legend, title, xlim, ylim, scale, log, logx, logy, filename, overplot, **kwargs)
236 if legend:
237 plt.legend()
--> 239 plt.tight_layout()
241 if filename != None:
242 plt.savefig(filename)
File ~/.local/lib/python3.8/site-packages/matplotlib/pyplot.py:2302, in tight_layout(pad, h_pad, w_pad, rect)
2300 @_copy_docstring_and_deprecators(Figure.tight_layout)
2301 def tight_layout(*, pad=1.08, h_pad=None, w_pad=None, rect=None):
-> 2302 return gcf().tight_layout(pad=pad, h_pad=h_pad, w_pad=w_pad, rect=rect)
File ~/.local/lib/python3.8/site-packages/matplotlib/figure.py:3197, in Figure.tight_layout(self, pad, h_pad, w_pad, rect)
3195 renderer = _get_renderer(self)
3196 with getattr(renderer, "_draw_disabled", nullcontext)():
-> 3197 kwargs = get_tight_layout_figure(
3198 self, self.axes, subplotspec_list, renderer,
3199 pad=pad, h_pad=h_pad, w_pad=w_pad, rect=rect)
3200 if kwargs:
3201 self.subplots_adjust(**kwargs)
File ~/.local/lib/python3.8/site-packages/matplotlib/tight_layout.py:320, in get_tight_layout_figure(fig, axes_list, subplotspec_list, renderer, pad, h_pad, w_pad, rect)
315 return {}
316 span_pairs.append((
317 slice(ss.rowspan.start * div_row, ss.rowspan.stop * div_row),
318 slice(ss.colspan.start * div_col, ss.colspan.stop * div_col)))
--> 320 kwargs = _auto_adjust_subplotpars(fig, renderer,
321 shape=(max_nrows, max_ncols),
322 span_pairs=span_pairs,
323 subplot_list=subplot_list,
324 ax_bbox_list=ax_bbox_list,
325 pad=pad, h_pad=h_pad, w_pad=w_pad)
327 # kwargs can be none if tight_layout fails...
328 if rect is not None and kwargs is not None:
329 # if rect is given, the whole subplots area (including
330 # labels) will fit into the rect instead of the
(...)
334 # auto_adjust_subplotpars twice, where the second run
335 # with adjusted rect parameters.
File ~/.local/lib/python3.8/site-packages/matplotlib/tight_layout.py:82, in _auto_adjust_subplotpars(fig, renderer, shape, span_pairs, subplot_list, ax_bbox_list, pad, h_pad, w_pad, rect)
80 if ax.get_visible():
81 try:
---> 82 bb += [ax.get_tightbbox(renderer, for_layout_only=True)]
83 except TypeError:
84 bb += [ax.get_tightbbox(renderer)]
File ~/.local/lib/python3.8/site-packages/matplotlib/axes/_base.py:4619, in _AxesBase.get_tightbbox(self, renderer, call_axes_locator, bbox_extra_artists, for_layout_only)
4617 if self.xaxis.get_visible():
4618 try:
-> 4619 bb_xaxis = self.xaxis.get_tightbbox(
4620 renderer, for_layout_only=for_layout_only)
4621 except TypeError:
4622 # in case downstream library has redefined axis:
4623 bb_xaxis = self.xaxis.get_tightbbox(renderer)
File ~/.local/lib/python3.8/site-packages/matplotlib/axis.py:1105, in Axis.get_tightbbox(self, renderer, for_layout_only)
1101 return
1103 ticks_to_draw = self._update_ticks()
-> 1105 self._update_label_position(renderer)
1107 # go back to just this axis's tick labels
1108 ticklabelBoxes, ticklabelBoxes2 = self._get_tick_bboxes(
1109 ticks_to_draw, renderer)
File ~/.local/lib/python3.8/site-packages/matplotlib/axis.py:2083, in XAxis._update_label_position(self, renderer)
2079 return
2081 # get bounding boxes for this axis and any siblings
2082 # that have been set by `fig.align_xlabels()`
-> 2083 bboxes, bboxes2 = self._get_tick_boxes_siblings(renderer=renderer)
2085 x, y = self.label.get_position()
2086 if self.label_position == 'bottom':
File ~/.local/lib/python3.8/site-packages/matplotlib/axis.py:1881, in Axis._get_tick_boxes_siblings(self, renderer)
1879 axis = getattr(ax, f"{axis_name}axis")
1880 ticks_to_draw = axis._update_ticks()
-> 1881 tlb, tlb2 = axis._get_tick_bboxes(ticks_to_draw, renderer)
1882 bboxes.extend(tlb)
1883 bboxes2.extend(tlb2)
File ~/.local/lib/python3.8/site-packages/matplotlib/axis.py:1085, in Axis._get_tick_bboxes(self, ticks, renderer)
1083 def _get_tick_bboxes(self, ticks, renderer):
1084 """Return lists of bboxes for ticks' label1's and label2's."""
-> 1085 return ([tick.label1.get_window_extent(renderer)
1086 for tick in ticks if tick.label1.get_visible()],
1087 [tick.label2.get_window_extent(renderer)
1088 for tick in ticks if tick.label2.get_visible()])
File ~/.local/lib/python3.8/site-packages/matplotlib/axis.py:1085, in <listcomp>(.0)
1083 def _get_tick_bboxes(self, ticks, renderer):
1084 """Return lists of bboxes for ticks' label1's and label2's."""
-> 1085 return ([tick.label1.get_window_extent(renderer)
1086 for tick in ticks if tick.label1.get_visible()],
1087 [tick.label2.get_window_extent(renderer)
1088 for tick in ticks if tick.label2.get_visible()])
File ~/.local/lib/python3.8/site-packages/matplotlib/text.py:910, in Text.get_window_extent(self, renderer, dpi)
907 raise RuntimeError('Cannot get window extent w/o renderer')
909 with cbook._setattr_cm(self.figure, dpi=dpi):
--> 910 bbox, info, descent = self._get_layout(self._renderer)
911 x, y = self.get_unitless_position()
912 x, y = self.get_transform().transform((x, y))
File ~/.local/lib/python3.8/site-packages/matplotlib/text.py:309, in Text._get_layout(self, renderer)
306 ys = []
308 # Full vertical extent of font, including ascenders and descenders:
--> 309 _, lp_h, lp_d = renderer.get_text_width_height_descent(
310 "lp", self._fontproperties,
311 ismath="TeX" if self.get_usetex() else False)
312 min_dy = (lp_h - lp_d) * self._linespacing
314 for i, line in enumerate(lines):
File ~/.local/lib/python3.8/site-packages/matplotlib/backends/backend_agg.py:259, in RendererAgg.get_text_width_height_descent(self, s, prop, ismath)
257 texmanager = self.get_texmanager()
258 fontsize = prop.get_size_in_points()
--> 259 w, h, d = texmanager.get_text_width_height_descent(
260 s, fontsize, renderer=self)
261 return w, h, d
263 if ismath:
File ~/.local/lib/python3.8/site-packages/matplotlib/texmanager.py:335, in TexManager.get_text_width_height_descent(self, tex, fontsize, renderer)
333 if tex.strip() == '':
334 return 0, 0, 0
--> 335 dvifile = self.make_dvi(tex, fontsize)
336 dpi_fraction = renderer.points_to_pixels(1.) if renderer else 1
337 with dviread.Dvi(dvifile, 72 * dpi_fraction) as dvi:
File ~/.local/lib/python3.8/site-packages/matplotlib/texmanager.py:271, in TexManager.make_dvi(self, tex, fontsize)
262 # Generate the dvi in a temporary directory to avoid race
263 # conditions e.g. if multiple processes try to process the same tex
264 # string at the same time. Having tmpdir be a subdirectory of the
(...)
268 # the absolute path may contain characters (e.g. ~) that TeX does
269 # not support.)
270 with TemporaryDirectory(dir=Path(dvifile).parent) as tmpdir:
--> 271 self._run_checked_subprocess(
272 ["latex", "-interaction=nonstopmode", "--halt-on-error",
273 f"../{texfile.name}"], tex, cwd=tmpdir)
274 (Path(tmpdir) / Path(dvifile).name).replace(dvifile)
275 return dvifile
File ~/.local/lib/python3.8/site-packages/matplotlib/texmanager.py:241, in TexManager._run_checked_subprocess(self, command, tex, cwd)
237 raise RuntimeError(
238 'Failed to process string with tex because {} could not be '
239 'found'.format(command[0])) from exc
240 except subprocess.CalledProcessError as exc:
--> 241 raise RuntimeError(
242 '{prog} was not able to process the following string:\n'
243 '{tex!r}\n\n'
244 'Here is the full report generated by {prog}:\n'
245 '{exc}\n\n'.format(
246 prog=command[0],
247 tex=tex.encode('unicode_escape'),
248 exc=exc.output.decode('utf-8'))) from exc
249 _log.debug(report)
250 return report
RuntimeError: latex was not able to process the following string:
b'lp'
Here is the full report generated by latex:
This is pdfTeX, Version 3.14159265-2.6-1.40.20 (TeX Live 2019/Debian) (preloaded format=latex)
restricted \write18 enabled.
entering extended mode
(../aba7304bf2416c0194a8a27472595813.tex
LaTeX2e <2020-02-02> patch level 2
L3 programming layer <2020-02-14>
(/usr/share/texlive/texmf-dist/tex/latex/base/article.cls
Document Class: article 2019/12/20 v1.4l Standard LaTeX document class
(/usr/share/texlive/texmf-dist/tex/latex/base/size10.clo))
(/usr/share/texlive/texmf-dist/tex/latex/type1cm/type1cm.sty)
! LaTeX Error: File `type1ec.sty' not found.
Type X to quit or <RETURN> to proceed,
or enter new name. (Default extension: sty)
Enter file name:
! Emergency stop.
<read *>
l.6 \usepackage
[utf8]{inputenc}^^M
No pages of output.
Transcript written on aba7304bf2416c0194a8a27472595813.log.Finally getting back to this. @webbjj, a few suggestions:
rieder
commented
2 years ago When testing clustertools with an AMUSE cluster, I run into unit problems:
!pip install amuse-framework amuse-masc
import clustertools as ctools
from amuse.units import units
from amuse.ext.masc import new_star_cluster
stars = new_star_cluster()
print(p.total_mass().in_(units.MSun))
cluster=ctools.StarCluster(ctype='amuse')
cluster.add_stars(p.x, p.y, p.z, p.vx, p.vy, p.vz, p.mass, p.key)
cluster.analyze(sortstars=True)IncompatibleUnitsException: Cannot express none in kg, the units do not have the same basesIt looks like the total mass of the cluster is not using an AMUSE unit here, where all the stellar masses are using a unit. These can then not be compared. Adding the stars with p.mass.value_in(units.MSun) seems to be a workaround, but I think when ctype is amuse it would be better to use AMUSE units (inherited from the first star added?) throughout.
rieder
commented
2 years ago I've created a script to compare Clustertools analysis to internal AMUSE analysis (and manual calculation), it seems they mostly agree but not entirely. Results vary with random realisations, but generally I find deviations in r10 (which is about 2 to 3 times what I would expect). Here's the script: https://gist.github.com/rieder/5b8c40f2cff8c932c7899a1f78096952
webbjj
commented
2 years ago @rieder - thanks for the feedback. For completeness:
cluster=cts.load_cluster('amuse',particles=stars,units='pckms',origin='cluster')
So here clustertools uses the value_in functionality to put the particles into the right units.
That being said, inheriting the units does seem like a common sense thing to do. It would probably be beneficial for anyone using astropy units too. Given how I do unit conversions to calculate certain quantities, such a feature might require some extensive coding. I will give it a quick look though now to see if I can implement it easily. What I can do easily is, if you specify units when you declare your StarCluster then I should tell add_star to check and see if the given arrays have units. (will do that asap)
@lwang-astro - I have not been able to reproduce your error, but some googling brought me to this page - https://github.com/matplotlib/matplotlib/issues/16911. Maybe you already know this, but for matplotlib versions > 3 something called cm-super is required. It is still strange that you can do latex labels outside of clustertools, so my only guess is that that there are certain labels or settings I use that trigger the need for cm-super. Does this sound like it could be the issue on your end? If so I should add a note in the requirements documentation.
lwang-astro
commented
2 years ago @webbjj Thank for the link. It provides the correct suggestion to solve the issue.
I found two packages are required on linux:
cm-super and dvipng
For Ubuntu, I use sudo apt install cm-super dvipng can solve the plotting error.
webbjj
commented
2 years ago Thanks @lwang-astro - I have made a note in the installation instructions for anyone else that might have this issue.
webbjj
commented
1 year ago @rieder - after some serious rewrites I think clustertools should now fully support AMUSE units. You would have to specify units='AMUSE' though when you initiate the StarCluster. The main issue was that I build all the arrays using numpy.append, which seemed to take an AMUSE VectorQuantity and turn it into an array of ScalarQuantities which many functions wouldn't work on. So I have just changed how the arrays are built when the input is VectorQuantity or ScalarQuantity. I have recreated all the tests I had previously written but for clusters in AMUSE units, and everything seems to check out. I also had to write some functions to give calculated variables AMUSE units if they are calculated in different units (e.g. galpy units).
@rieder and @lwang-astro - I think I am now all caught up with your comments so far. Please let me know if I have missed something and you are waiting on me. Looking forward to more feedback if you have any. This has helped the code tremendously. Thanks so much.
rieder
commented
1 year ago Thanks @webbjj, great work! I will have time to continue the review next week :).
lwang-astro
commented
1 year ago @webbjj Sorry for the late of reply. Recently I am busy with teaching. Now I am continuing to test the example notebook. I use the newest pip version of clustertools.
I got an error with
cluster.rcore(plot=True)
print(cluster.rc)```
The error message:UnboundLocalError Traceback (most recent call last)
Input In [40], in
File ~/.local/lib/python3.8/site-packages/clustertools/cluster/cluster.py:3151, in rcore(self, method, nneighbour, mfrac, projected, plot, **kwargs)
3118 def ckin(
3119 self,
3120 mmin=None,
(...)
3132 projected=False,
3133 ):
3134 """
3135 NAME: Find the kinematic concentration parameter ck
3136
3137 - see Bianchini et al. 2018, MNRAS, 475, 96
3138
3139 Parameters
3140 ----------
3141 cluster : class
3142 StarCluster instance
3143 mmin/mmax : float
3144 specific mass range
3145 nmass :
3146 number of mass bins used to calculate alpha
3147 rmin/rmax :
3148 specific radial range
3149 vmin/vmax : float
3150 specific velocity range
-> 3151 emin/emax : float
3152 specific energy range
3153 kwmin/kwmax : int
3154 specific stellar evolution type range
3155 npop : int
3156 population number
3157 indx : bool
3158 specific subset of stars
3159 projected : bool
3160 use projected values (default: False)
3161
3162 Returns
3163 -------
3164 ck : float
3165 kinematic concentration
3166
3167 Other Parameters
3168 ----------------
3169 kwargs : str
3170 key words for plotting
3171
3172 History
3173 -------
3174 2020
3175 """
3176 c_k = ckin(
3177 self,
3178 mmin=mmin,
(...)
3190 projected=projected,
3191 )
3193 self.ck=c_k
File ~/.local/lib/python3.8/site-packages/clustertools/analysis/functions.py:2146, in rcore(cluster, method, nneighbour, mfrac, projected, plot, kwargs) 2122 def rcore( 2123 cluster, 2124 method='casertano', (...) 2129 kwargs 2130 ): 2131 """Calculate core radius of the cluster 2132 -- The default method (method='casertano') follows Casertano, S., Hut, P. 1985, ApJ, 298, 80 to find the core 2133 -- An alternative metrhod (method=='isothermal') assumes the cluster is an isothermal sphere the core radius is where density drops to 1/3 central value 2134 --- For projected core radius, the core radius is where the surface density profile drops to 1/2 the central value 2135 --- Note that the inner mass fraction of stars used to calculate central density is set by mfrac (default 0.1 = 10%) 2136 2137 Parameters 2138 ---------- 2139 cluster : class 2140 StarCluster instance 2141 method : string 2142 method of calculating the core radius of a star cluster (default 'casertano') 2143 if method =='casertano': 2144 nneighbour : int 2145 number of neighbours for calculation local densities -> 2146 if method=='isothermal': 2147 mfrac : float 2148 inner mass fraction to be used to establish the central density 2149 projected : bool 2150 use projected values (default: False) 2151 plot : bool 2152 plot the density profile and mark the core radius of the cluster (default: False) 2153 Returns 2154 ------- 2155 rc : float 2156 core radius 2157 2158 Other Parameters 2159 ---------------- 2160 None 2161 2162 History 2163 ------- 2164 2021 - Written - Webb (UofT) 2165 2022 - Written - Webb (UofT) - add method='casertano' 2166 """ 2167 cluster.save_cluster() 2168 units0,origin0, rorder0, rorder_origin0 = cluster.units0,cluster.origin0, cluster.rorder0, cluster.rorder_origin0
UnboundLocalError: local variable 'nrad' referenced before assignment
My apologies @lwang-astro , it looks like I broke the plotting feature when adding your suggested method of calculating rcore. Should be fixed now in the newest pip (clustertools-1.0.4) or git hub.
webbjj
commented
1 year ago Actually lets do 1.0.5. I didn't upload 1.0.4 correctly.
dfm
commented
1 year ago 👋 @lwang-astro, @rieder, @webbjj — Hi folks, I wanted to check in here because we're recently switched to a "tracks" model here at JOSS and I'm now the managing editor of the track where this submission has been assigned. It looks like there hasn't been much activity on this review in a while - can you update me on what the status is and if there are any blockers in place that I can help with? Thanks!
webbjj
commented
1 year ago Hello all. I believe I am caught up with all of @lwang-astro and @rieder 's comments. However please let me know if you are waiting on me for something.
lwang-astro
commented
1 year ago @webbjj @dfm Sorry for the late of reply. Now I am continuing to check the tool. For the previous problem of plotting bug.
Latex Error: File 'type1cm.sty' not found.
I test the code on the other machine and found that only installing cm-super and dvipng are not enough (for ubuntu: sudo apt install cm-super dvipng). The texlive and texlive-extra also needed to be installted together. Probably 'type1cm.sty' is from texlive-extra. I am not sure which function in the clustertool requires 'type1cm.sty'. Generally the default matplotlib should support most latex symbols without texlive-extra package.
I have tested the samples in Example Notebooks except the last one "Orbits and Tidal Tails" , since I have no available sample data to test it directly. For other example sources, they work if the plotting problem is solved.
webbjj
commented
1 year ago Thanks @lwang-astro !
lwang-astro
commented
1 year ago @webbjj For ubuntu, I think that installation of texlive is necessay to show latex symbol if matplotlib require latex compiler. For some reason which I don't know in detail, some ploting function in clustertools require type1cm.sty, which is only available in texlive-extra. Thus, to make the plotting function work correctly, both are needed to be installed.
webbjj
commented
1 year ago Thanks @lwang-astro - ok I have added a note to the installation instructions. However I also "maybe" have fixed the issue. I was relying on seaborn for a colour palette, and seaborn changes some of the matplotlib parameters. There was also a galpy plotting function that was changing matplotlib parameters too. So I have removed these dependencies and try to import matplotlib last everywhere. Its hard to tell, because I can't reproduce the error. But as far as I can tell the default matplotlib parameters are now the same before and after importing clustertools.
rieder
commented
1 year ago Sorry for taking a while to get back to this... I'm looking at the tests now, and these seem to fail, with varying errors:
from limepy import limepy fails)I am not sure if these are all the errors in the tests.
webbjj
commented
1 year ago Hi @rieder , thanks for the feedback.
rieder
commented
1 year ago Hi,
Maybe you could make the tests for amuse/limepy/astropy optional, depending on whether these packages are installed or not (e.g. with try... except ImportError)?
rieder
commented
1 year ago ImportError while importing test module '/Users/rieder/Review/clustertools/tests/test_loaders.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/local/Library/Frameworks/Python.framework/Versions/3.10/lib/python3.10/importlib/__init__.py:126: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
tests/test_loaders.py:13: in <module>
from limepy import limepy
E ImportError: cannot import name 'limepy' from 'limepy' (/Users/rieder/Review/clustertools/env/lib/python3.10/site-packages/limepy/__init__.py)this is an example of an error I get from limepy not importing correctly in one of the tests. It prevents the tests from running at all.
Submitting author: !--author-handle-->@webbjj<!--end-author-handle-- (Jeremy Webb) Repository: https://github.com/webbjj/clustertools Branch with paper.md (empty if default branch): Version: 1.1 Editor: !--editor-->@christinahedges<!--end-editor-- Reviewers: @lwang-astro, @rieder Archive: 10.5281/zenodo.7941851
Status
Status badge code:
Reviewers and authors:
Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)
Reviewer instructions & questions
@lwang-astro & @rieder, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @christinahedges know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Checklists
📝 Checklist for @rieder
📝 Checklist for @lwang-astro