-
Notifications
You must be signed in to change notification settings - Fork 64
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
Doc review 2023 03 round 01 #869
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of small comments/questions from me. Thanks for kicking this effort off, @gonuke!
doc/install/index.rst
Outdated
If you plan to use OpenMC_, you can follow their installation instructions with | ||
DAGMC built-in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to link to the instructions here: https://docs.openmc.org/en/stable/quickinstall.html
doc/install/dependencies.rst
Outdated
@@ -79,45 +79,33 @@ Redhat linux users can do likewise with: | |||
MOAB installation | |||
~~~~~~~~~~~~~~~~~ | |||
|
|||
As of DAGMC version 3.1, MOAB version 5.1.0 or higher is required. The following | |||
As of DAGMC version 3.2, MOAB version 5.3.0 or higher is required. The following |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this again? My recollection is that we could still build the latest DAGMC with MOAB v5.1.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question... it may be <5.3.0 ? I'll check...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just built DAGMC develop and v3.2 against MOAB v5.1.0 and didn't run into any issues. Maybe we can leave this for now.
caa1948
to
2cc4b7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small suggestions and I think that we're looking good here!
9a4aab9
to
9c5a15b
Compare
2204d39
to
b652fe7
Compare
8606700
to
da04814
Compare
This is failing windows tests @pshriwise - but it's probably not because of this PR... I've addressed your questions, so let me know if you think it's ready to merge. |
Yes, I have also faced a similar problem in my PR #860. I am not sure, but I am assuming there is a bug in MOAB. |
Its a build failure due to Gtest bizzarely
|
Dear @makeclean, I have updated gtest to v1.13.0 and found the same error. Here is the log. |
Dear @gonuke, I believe that some of the resolved changes might have been accidentally dropped in the latest force-push. Could you please check? |
Oh no! You may be right! Let me see if I can recover them manually |
9af046e
to
3f459ce
Compare
3f459ce
to
96e4aed
Compare
Converted this to draft until #880 is merged |
b53e1ad
to
ef287dd
Compare
This has been rebased & tested (over-tested for now since it's only a doc change) and ready to merge once @pshriwise approves changes. |
Co-authored-by: Patrick Shriwise <[email protected]>
25c9df6
to
c3f4eb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for diving into documentation updates, @gonuke!
What do you think about instructing users to rely on a package manager installation of HDF5 via apt
or conda
rather than recommending building from source?
tmp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope - definitely not!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small quibble...
@@ -79,45 +79,32 @@ Redhat linux users can do likewise with: | |||
MOAB installation | |||
~~~~~~~~~~~~~~~~~ | |||
|
|||
As of DAGMC version 3.1, MOAB version 5.1.0 or higher is required. The following | |||
As of DAGMC version 3.2, a MOAB version between 5.1.0 and 5.3.0 is required. The following |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be between 5.10
and 5.3.0
again? I thought we could also support newer versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are significant changes in v5.4.0 that are causing some build fail. We need to resolve this issue before proceeding to v5.4 MOAB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Perhaps we could clarify that the restriction to 5.4.0 is only for Windows if that is the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our CI is stuck on 5.3.0 until we can figure out all the issues. It's a limitation for Ubuntu as well for 20.04 and older - seems to work on 22.04, though.
With all those caveats, it's seems best to stick with 5.3.0 for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough! Strange though... hopefully we can figure it out someday.
@ahnaf-tahmid-chowdhury I'm happy with this if you are. If you don't have any further comments, feel free to merge! ... if you have permissions, that is. Otherwise I can take care of it. |
I'm also satisfied with it. After my most recent active PR gets merged, I'm considering creating a new PR to address the latest version of MOAB. It appears that I inadvertently created a forked repo from another fork last time. |
Description
First round of incremental documentation improvements in Spring 2023.
Motivation and Context
Documentation has become confusingly out of date and needs to be improved.
This round focuses on the home page and installation info.