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

Doc review 2023 03 round 01 #869

Merged
merged 12 commits into from
Aug 20, 2023
Merged

Conversation

gonuke
Copy link
Member

@gonuke gonuke commented Mar 16, 2023

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.

Copy link
Member

@pshriwise pshriwise left a 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/dependencies.rst Outdated Show resolved Hide resolved
Comment on lines 6 to 8
If you plan to use OpenMC_, you can follow their installation instructions with
DAGMC built-in.
Copy link
Member

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

@@ -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
Copy link
Member

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.

Copy link
Member Author

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...

Copy link
Member

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.

Copy link
Member

@pshriwise pshriwise left a 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!

doc/install/dependencies.rst Outdated Show resolved Hide resolved
doc/install/index.rst Outdated Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
@gonuke
Copy link
Member Author

gonuke commented Jul 20, 2023

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.

@ahnaf-tahmid-chowdhury
Copy link
Member

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.

@makeclean
Copy link
Contributor

Its a build failure due to Gtest bizzarely

D:\a\DAGMC\DAGMC\src\gtest\gtest-1.8.0\src/gtest-internal-inl.h(641,31): error C2039: 'SetUpTestCaseFunc': is not a member of 'testing::Test' [D:\a\DAGMC\build\src\gtest\gtest.vcxproj]

@ahnaf-tahmid-chowdhury
Copy link
Member

Its a build failure due to Gtest bizzarely

D:\a\DAGMC\DAGMC\src\gtest\gtest-1.8.0\src/gtest-internal-inl.h(641,31): error C2039: 'SetUpTestCaseFunc': is not a member of 'testing::Test' [D:\a\DAGMC\build\src\gtest\gtest.vcxproj]

Dear @makeclean, I have updated gtest to v1.13.0 and found the same error. Here is the log.

@ahnaf-tahmid-chowdhury
Copy link
Member

Dear @gonuke, I believe that some of the resolved changes might have been accidentally dropped in the latest force-push. Could you please check?

@gonuke
Copy link
Member Author

gonuke commented Jul 22, 2023

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

@gonuke
Copy link
Member Author

gonuke commented Aug 13, 2023

Converted this to draft until #880 is merged

@gonuke gonuke force-pushed the doc_review_2023_03 branch 3 times, most recently from b53e1ad to ef287dd Compare August 16, 2023 01:02
@gonuke gonuke marked this pull request as ready for review August 16, 2023 01:05
@gonuke
Copy link
Member Author

gonuke commented Aug 16, 2023

This has been rebased & tested (over-tested for now since it's only a doc change) and ready to merge once @pshriwise approves changes.

Copy link
Member

@pshriwise pshriwise left a 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
Copy link
Member

Choose a reason for hiding this comment

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

Should this be here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope - definitely not!!!

Copy link
Member

@pshriwise pshriwise left a 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
Copy link
Member

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.

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.

Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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.

@pshriwise
Copy link
Member

pshriwise commented Aug 19, 2023

@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.

@ahnaf-tahmid-chowdhury
Copy link
Member

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.

@pshriwise pshriwise merged commit b3e2b13 into svalinn:develop Aug 20, 2023
2 checks passed
@gonuke gonuke deleted the doc_review_2023_03 branch August 21, 2023 00:10
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.

5 participants