docs: overall review

[PipKat]

I wasn't very comfortable editing the content in ...heart/writer/custom_keywords and so only made minimal changes to files in this folder. Also in the vtkutils.py file, there is use of "master" and "slave" terminology, which Ansys made an effort to scrub from its products several years ago. I'm not sure if we can or should scrub it from here.

Lastly, there is a typo in create_element_solid_keyword.

You can review and merge this PR. Let me know if the schedule for making this library public allows time for me to skim the rendered doc first though. It's a complicated subject and relatively large repo, so I'm sure there are some things I missed and some formatting errors that should be corrected.

[mhoeijm]

In-Progress. @jorgepiloto Can you fix the problem causing the failure? I still have about 1/2 the examples to review and the API doc.

Hi @PipKat, the failing action was related to the PR title, it was only valid after you edited it. Somehow the action wasn't properly re-run after editing the PR title though. That only seems to happen when a commit is made. So I made two dummy commits to get the action to rerun, it passes now.

[jorgepiloto]

Thanks for opening this, @PipKat. Looks like @mhoeijm fixed the issue.

[PipKat]

@mhoeijm and @jorgepiloto I'm not familiar with this doc building error: Extension error:
All "sphinx_gallery_start_ignore" flags must have a matching "sphinx_gallery_end_ignore" flag!

Do you think you can get the doc build running on Monday morning? After a quick perusal of the doc in the HTML archive, I could move on to the final stage of reviewing the API reference content. Thanks!

[mhoeijm]

@jorgepiloto @PipKat, we are working on executing the examples in the doc build, so that should remove most of the sphinx_gallery_start_ignore / sphinx_gallery_end_ignore blocks. I checked all ignore blocks in this branch, and to me they all have an ending, so not entirely sure why it is throwing an error here.

[jorgepiloto]

Same, still investigating what changed in this branch for the CI/CD to fail.

[jorgepiloto]

Running:

grep -E "sphinx_gallery_start_ignore|sphinx_gallery_end_ignore" -r examples | wc -l

verifies that there are 30 of these annotations, all of them paired.

[jorgepiloto]

I pushed some commits to fix some syntax that may be causing the issue in d34d917 and f5ab60f

[jorgepiloto]

Tracing back the CI/CD runs, it must be something introduced in commit 90b435f. Last commit before that one passed, see 7597df2

[jorgepiloto]

@PipKat, commit 360c63c should fix the issue.

[mhoeijm]

I wasn't very comfortable editing the content in ...heart/writer/custom_keywords and so only made minimal changes to files in this folder. Also in the vtkutils.py file, there is use of "master" and "slave" terminology, which Ansys made an effort to scrub from its products several years ago. I'm not sure if we can or should scrub it from here.

Lastly, there is a typo in create_element_solid_keyword.

You can review and merge this PR. Let me know if the schedule for making this library public allows time for me to skim the rendered doc first though. It's a complicated subject and relatively large repo, so I'm sure there are some things I missed and some formatting errors that should be corrected.

Thank you for the detailed review @PipKat! I addressed two of your comments with 721482f and a65a081.

Regarding ...heart/writer/custom_keywords: actually these are keywords that originate from PyDYNA's keyword module (https://github.com/ansys/pydyna/tree/main/src/ansys/dyna/core/keywords/keyword_classes). Some of these were auto-generated from other source code - so if there are any mistakes in the docstrings than that probably comes from there. There are a few keywords with bugs, or that cannot be auto generated, and in order to use those we included them here as custom_keywords. I don't think that we can or should expose these in the documentation since it will essentially be a duplicate from PyDYNA.

I would say that we can open issues for anything that we still need to resolve prior to release, and we can merge this into main. That will allow us to continue working on modifying the examples such that they can be executed during doc build. @jorgepiloto, what do you think?

Planning wise: there are some other open issues that we need to resolve, but I think that 1 or max 2 weeks to finalize before public release should be doable. So @PipKat we have a little time to work on the details.

[jorgepiloto]

I would say that we can open issues for anything that we still need to resolve prior to release, and we can merge this into main. That will allow us to continue working on modifying the examples such that they can be executed during doc build. @jorgepiloto, what do you think?

I agree about this. We can merge the major work @PipKat did here so we can progress on other pull-request. If we can extend this work before the release, that's perfect.

[PipKat]

@mhoeijm I'll let you handle conflict resolution given that you are working in the examples. I'll create an issue for myself to remove the View Project Documentation card from the Contribute section for users. I'll do this when doing a skim of the doc generated once this PR is merged.

[PipKat]

Regarding ...heart/writer/custom_keywords: actually these are keywords that originate from PyDYNA's keyword module (https://github.com/ansys/pydyna/tree/main/src/ansys/dyna/core/keywords/keyword_classes). Some of these were auto-generated from other source code - so if there are any mistakes in the docstrings than that probably comes from there. There are a few keywords with bugs, or that cannot be auto generated, and in order to use those we included them here as custom_keywords. I don't think that we can or should expose these in the documentation since it will essentially be a duplicate from PyDYNA.

I would say that we can open issues for anything that we still need to resolve prior to release, and we can merge this into main. That will allow us to continue working on modifying the examples such that they can be executed during doc build. @jorgepiloto, what do you think?

Planning wise: there are some other open issues that we need to resolve, but I think that 1 or max 2 weeks to finalize before public release should be doable. So @PipKat we have a little time to work on the details.

@mhoeijm Do we need to reset to the original source files for the custom keywords or will this be auto-generated again from the other source code? Or does it not matter short-term because they aren't exposed in the PyHeart documentation?

[mhoeijm]

@mhoeijm I'll let you handle conflict resolution given that you are working in the examples. I'll create an issue for myself to remove the View Project Documentation card from the Contribute section for users. I'll do this when doing a skim of the doc generated once this PR is merged.

@PipKat, @jorgepiloto Done. A good thing I didn't modify any of the other examples! I hope I didn't revert some of your earlier changes in the four example files that now end with _pr.py. The following might have been affected by the conflict resolution:

• preprocess_truncated_LV_pr.py
• example_atrial_fiber_pr.py
• download_case_pr.py
• demo_material_pr.py

[mhoeijm]

Regarding ...heart/writer/custom_keywords: actually these are keywords that originate from PyDYNA's keyword module (https://github.com/ansys/pydyna/tree/main/src/ansys/dyna/core/keywords/keyword_classes). Some of these were auto-generated from other source code - so if there are any mistakes in the docstrings than that probably comes from there. There are a few keywords with bugs, or that cannot be auto generated, and in order to use those we included them here as custom_keywords. I don't think that we can or should expose these in the documentation since it will essentially be a duplicate from PyDYNA.
I would say that we can open issues for anything that we still need to resolve prior to release, and we can merge this into main. That will allow us to continue working on modifying the examples such that they can be executed during doc build. @jorgepiloto, what do you think?
Planning wise: there are some other open issues that we need to resolve, but I think that 1 or max 2 weeks to finalize before public release should be doable. So @PipKat we have a little time to work on the details.

@mhoeijm Do we need to reset to the original source files for the custom keywords or will this be auto-generated again from the other source code? Or does it not matter short-term because they aren't exposed in the PyHeart documentation?

Since it is only the docstrings that seem to be affected I don't think that there is a need to revert. Ideally everything under custom_keywords will be removed once properly fixed in PyDYNA. Hence I would avoid putting any effort into that. And as you said, they won't be exposed directly anyway.

[PipKat]

@mhoeijm I'll let you handle conflict resolution given that you are working in the examples. I'll create an issue for myself to remove the View Project Documentation card from the Contribute section for users. I'll do this when doing a skim of the doc generated once this PR is merged.

@PipKat, @jorgepiloto Done. A good thing I didn't modify any of the other examples! I hope I didn't revert some of your earlier changes in the four example files that now end with _pr.py. The following might have been affected by the conflict resolution:

• preprocess_truncated_LV_pr.py
• example_atrial_fiber_pr.py
• download_case_pr.py
• demo_material_pr.py

@mhoeijm I'll make a note to myself to read though those four examples closer than the others when I do a skim of the rendered doc.