Skip to content
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

420 accretion disk tutorial #506

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

Basdorsman
Copy link
Contributor

The tutorial is added and ready for checking. I compiled the documentation locally and that appeared to work fine.

@Basdorsman Basdorsman self-assigned this Jan 8, 2025
@Basdorsman Basdorsman linked an issue Jan 8, 2025 that may be closed by this pull request
@Basdorsman Basdorsman requested a review from thjsal January 8, 2025 11:08
@Basdorsman
Copy link
Contributor Author

@thjsal any idea why this test is failing?

@thjsal
Copy link
Contributor

thjsal commented Jan 8, 2025

I have do not know why this happens (at least right now), but this seems to be the error message that comes when GitHub is installing X-PSI:

Traceback (most recent call last):
  File "/home/runner/work/xpsi/xpsi/setup.py", line 77, in <module>
Operating system: linux
GSL version: 2.7.1
Warning: the rayXpanda package cannot be imported. Using fallback implementation.
    import rayXpanda
ModuleNotFoundError: No module named 'rayXpanda'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/runner/work/xpsi/xpsi/setup.py", line 82, in <module>
    sub.call(['%s'%CC,
  File "/home/runner/micromamba/envs/xpsipy3/lib/python3.[9](https://github.com/xpsi-group/xpsi/actions/runs/12669137815/job/35306004548#step:8:10)/subprocess.py", line 349, in call
    with Popen(*popenargs, **kwargs) as p:
  File "/home/runner/micromamba/envs/xpsipy3/lib/python3.9/subprocess.py", line 951, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/home/runner/micromamba/envs/xpsipy3/lib/python3.9/subprocess.py", line 1837, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'gcc-[10](https://github.com/xpsi-group/xpsi/actions/runs/12669137815/job/35306004548#step:8:11)'

@thjsal
Copy link
Contributor

thjsal commented Jan 8, 2025

Probably some default packages/compilers that GitHub uses have been upgraded, and the new versions fail for some reason.

@drannawatts
Copy link
Member

Gcc issue associated with the new ubuntu version? Seems like the one they are in the middle of flipping to as ubuntu-latest uses gcc-11 or 12 rather than 10 (since it's not finding gcc-10)

@thjsal
Copy link
Contributor

thjsal commented Jan 8, 2025

I guess so. Then probably just need to change this line (and possibly also in other workflows):

@drannawatts
Copy link
Member

Looks like we'll have to go to gcc-12...

actions/runner-images#10636

@thjsal
Copy link
Contributor

thjsal commented Jan 16, 2025

I looked at this tutorial quickly and it looks overall nice! Here are my comments:

  1. I suggest to re-run the tutorial with v3 of X-PSI so that it will show this number in the docs as well (I already checked that it should run with the latest version).

  2. It seems to me that the tutorial mostly goes through how to write the Disk class stuff. It might be a bit scary for a user who would just want to apply this instead of developing it. Maybe it could be early pointed out that this class and necessary modules already exist somewhere (at least in the examples), and put a link to where it exists. It would be nice to be able to just import an already existing Disk class.... but sure, maybe the proper place for this class would then be in the xpsi folder in instead of the examples (perhaps this is something to be done later).

  3. I would rather use split=True option with these atmosphere data since it is much faster to compute (and equally accurate). Or at least mention there that this is recommended for a more efficient analysis.

  4. I think photo accepts should probably say photosphere accepts (after the code block 12).

Corrected some small typos and points of English.
Copy link
Member

@drannawatts drannawatts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've corrected a few small typos. Looks good though!

I think Tuomo's suggestions are good. I also wondered whether there is any way to have hyperlinks to the various papers that are cited, in the notebooks? Maybe this is not easy though.

You mention in the middle of the tutorial the 'distance to the disk'. We are assuming that this is identical in our modelling to distance to star, I think? I'd suggest adding a note to make this clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accretion disk Tutorial
3 participants