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

added symlinks for tardis_example.yml #1562

Merged
merged 4 commits into from
May 12, 2021
Merged

added symlinks for tardis_example.yml #1562

merged 4 commits into from
May 12, 2021

Conversation

parikshit14
Copy link
Contributor

@parikshit14 parikshit14 commented May 11, 2021

Added Symlinks for tardis_example wherever they were needed

Description
Replaced the original file with symlinks:

  1. tardis/docs/using/components/models/examples/tardis_example.yml

placed the original tardis_example.yml in tardis/docs

also added a symlink in tardis/docs/quickstart/ as it is generally used in quickstart.ipynb

Motivation and context

solves issue #1561

Type of change

  • Bug fix.
  • New feature.
  • Breaking change.
  • None of the above.

Checklist

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
    • (optional) I have built the documentation on my fork following the instructions.
  • I have assigned and requested two reviewers for this pull request.
    @smithis7 @wkerzendorf kindly verify the changes

@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #1562 (39a3202) into master (e3a35bb) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1562   +/-   ##
=======================================
  Coverage   68.24%   68.24%           
=======================================
  Files          73       73           
  Lines        6373     6373           
=======================================
  Hits         4349     4349           
  Misses       2024     2024           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3a35bb...39a3202. Read the comment docs.

@@ -0,0 +1 @@
/home/parikshit/Desktop/tardis/docs/tardis_example.yml
Copy link
Member

@atharva-2001 atharva-2001 May 11, 2021

Choose a reason for hiding this comment

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

I guess you should use relative paths here. In this case, just ../tardis_example.yml should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for rectifying the mistakes I really don't know how I missed them. Thanks again

@isaacgsmith
Copy link
Member

Good start! A few things:

  • docs/research/code_comparison/plasma_compare/tardis_example.yml is actually a different YAML file from the one elsewhere in the documentation, so we want to leave it how it was. I can see where having it named tardis_example.yml could cause confusion, and that may be something to rename in the future.
  • There are several other occurrences of the YAML file throughout the documentation. They are all currently symlinks that direct to docs/using/components/models/examples/tardis_example.yml (so in this PR, you should only need to replace the actual file with a symlink once, and everything else will just be changing symlinks like you did with quickstart). To go about finding them all, you will want to build the documentation locally, and see where the notebooks fail to run (which will be the result of a broken symlink), and fix the symlink. You'll want to repeat this until the docs build properly. @epassaro once showed me a more efficient way to locate all broken symlinks, but I don't recall what it was (maybe he can comment how to do it).

@parikshit14
Copy link
Contributor Author

Good start! A few things:

  • docs/research/code_comparison/plasma_compare/tardis_example.yml is actually a different YAML file from the one elsewhere in the documentation, so we want to leave it how it was. I can see where having it named tardis_example.yml could cause confusion, and that may be something to rename in the future.

ok, I thought they were the same. Just checked and found the difference, no problem I will revert this back to normal.

  • There are several other occurrences of the YAML file throughout the documentation. They are all currently symlinks that direct to docs/using/components/models/examples/tardis_example.yml (so in this PR, you should only need to replace the actual file with a symlink once, and everything else will just be changing symlinks like you did with quickstart). To go about finding them all, you will want to build the documentation locally, and see where the notebooks fail to run (which will be the result of a broken symlink), and fix the symlink. You'll want to repeat this until the docs build properly. @epassaro once showed me a more efficient way to locate all broken symlinks, but I don't recall what it was (maybe he can comment how to do it).

ok, so all the existing symlinks(present originally in this repository) are reference from docs/using/components/models/examples/tardis_example.yml and not from docs/research/code_comparison/plasma_compare/tardis_example.yml. correct me if I am wrong! because this can again lead to a lot of confusion.

@isaacgsmith
Copy link
Member

ok, so all the existing symlinks(present originally in this repository) are reference from docs/using/components/models/examples/tardis_example.yml and not from docs/research/code_comparison/plasma_compare/tardis_example.yml. correct me if I am wrong! because this can again lead to a lot of confusion.

Right. The issue I posted is only concerned with the file that was under docs/using/components/models/examples/tardis_example.yml, so you just need to change symlinks that point to docs/using/components/models/examples/tardis_example.yml so that they point to docs/tardis_example.yml.

@parikshit14
Copy link
Contributor Author

I tried to test the changes you recommended in the second part but apparently the symlinks are not broken i.e. the symlinks which are now pointing to the symlink of docs/tardis_example.yml (symlink -> symlink -> docs/tardis_example.yml) are working completely fine. I checked them using the notebooks given with them respectively and they work the same as they were working before.
But still, if you require I can update them to the current situation

@isaacgsmith
Copy link
Member

isaacgsmith commented May 11, 2021

I tried to test the changes you recommended in the second part but apparently the symlinks are not broken i.e. the symlinks which are now pointing to the symlink of docs/tardis_example.yml (symlink -> symlink -> docs/tardis_example.yml) are working completely fine. I checked them using the notebooks given with them respectively and they work the same as they were working before.

But still, if you require I can update them to the current situation

I see why that is. Technically, it would be fine, as the symlinks point to another symlink that points to the proper file, but I think it would be better to change them so they point directly to the proper place. I think you can find all of them by searching for tardis_example.yml in the TARDIS code.

@parikshit14
Copy link
Contributor Author

Updated as suggested!

@isaacgsmith
Copy link
Member

Looks really good! This is my fault as I did not put this in the original issue, but as I was reviewing this I noticed that there is a link in docs/quickstart/quickstart.ipynb (3rd code cell) that points to the old location. That's the last link that points to the old location that I found, so if you can change that one I will approve the PR. Sorry again for missing that link in the original issue.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@parikshit14
Copy link
Contributor Author

Looks really good! This is my fault as I did not put this in the original issue, but as I was reviewing this I noticed that there is a link in docs/quickstart/quickstart.ipynb (3rd code cell) that points to the old location. That's the last link that points to the old location that I found, so if you can change that one I will approve the PR. Sorry again for missing that link in the original issue.

no problem, I also missed it while I was testing quickstart.ipynb !!

Copy link
Member

@isaacgsmith isaacgsmith left a comment

Choose a reason for hiding this comment

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

Perfect!

@andrewfullard andrewfullard self-requested a review May 12, 2021 16:23
@andrewfullard andrewfullard merged commit 92b7e1d into tardis-sn:master May 12, 2021
@andrewfullard andrewfullard linked an issue May 12, 2021 that may be closed by this pull request
@isaacgsmith isaacgsmith mentioned this pull request May 13, 2021
10 tasks
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
* added symlinks for tardis_example.yml

* updating symlinks

* symlink corrections

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

Successfully merging this pull request may close these issues.

Moving Config File Under docs/
4 participants