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

Add Cubit example to the immersed_fem demo #2116

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

GregVernon
Copy link

@GregVernon GregVernon commented Jun 18, 2021

I noticed that Firedrake's manual mentioned it supports Exodus files, but when I tried changing the immersed_fem.py demo to

mesh = Mesh('immersed_domain.e')

I got errors saying I needed to build with Exodus support using the --download-exodusii argument. But the firedrake-install script doesn't accept this argument -- so I've added that optional argument to the install script.

I then proceeded with creating a Cubit-based workflow to build the mesh for this problem, for both tri and quad elements. I've not been able to verify the documentation in immersed_fem.py renders correctly, could use some advice for how to test...

--
Greg Vernon
Director of Product Management, Coreform LLC

@wence-
Copy link
Contributor

wence- commented Jun 19, 2021

Hi Greg, thanks for this demo!

We used to install ExodusII by default, but we found it caused a bunch of headaches in the installer, so dropped it by default in #1747.

Passing extra flags to the PETSc build is actually possible, you run PETSC_CONFIGURE_OPTIONS=--download-exodusii firedrake-install .... This is perhaps not very discoverable (firedrake-install --help says it, but the download/install page on the website does not). Perhaps we should add a section there about custom PETSc options.

I then proceeded with creating a Cubit-based workflow to build the mesh for this problem, for both tri and quad elements. I've not been able to verify the documentation in immersed_fem.py renders correctly, could use some advice for how to test...

If you have a working firedrake, then do firedrake-update --documentation-dependencies and build the docs in the firedrake/docs subdirectory with make html (see https://www.firedrakeproject.org/download.html#building-the-documentation). You'll probably want to add a link to this demo from the docs/source/documentation.rst page (likely in the "advanced demos" section).

Thanks!

@GregVernon
Copy link
Author

GregVernon commented Jun 19, 2021

Hi @wence-

We used to install ExodusII by default, but we found it caused a bunch of headaches in the installer, so dropped it by default in #1747.

Weird that it's causing headaches, that's certainly an issue!

Passing extra flags to the PETSc build is actually possible, you run PETSC_CONFIGURE_OPTIONS=--download-exodusii firedrake-install .... This is perhaps not very discoverable (firedrake-install --help says it, but the download/install page on the website does not). Perhaps we should add a section there about custom PETSc options.

It wasn't very discoverable to me, I looked around for several hours trying to figure it out. My confusion came from the webpage saying that firedrake supported Exodus, so I (initially) expected this to be at the firedrake-install level -- not "hacking" a dependency installation (I know it's not really a hack, but I wasn't sure if just adding Exodus to PETSc would mean that firedrake would recognize Exodus). To this point, the firedrake-install script does say:

The installer will ensure that the required configuration options are
passed to PETSc. In addition, any configure options which you provide
in the PETSC_CONFIGURE_OPTIONS environment variable will be
honoured.

But there's a difference between "honoured" and "supported".

So just thinking out-loud here, what would you think would be the best approach?

  1. Change my commit from default to an optional firedrake-install argument, specifically for Exodus

    • For example:
    if not options["minimal_petsc"]:    
      petsc_options.add("--with-zlib")    
      # File formats    
      petsc_options.add("--download-netcdf")    
      petsc_options.add("--download-pnetcdf")    
      if options["with_exodus"]
        petsc_options.add("--download-exodusii")
    
  2. Expand the help documentation for firedrake-install to include "common" PETSc flags that are supported + add webpage documentation + add description to the Cubit demo

  3. Other?

@dham
Copy link
Member

dham commented Jul 7, 2021

Hmm. What about if we adopt -- as an argument separator and then pass all options after -- straight through to PETSc. We could also modify our error message for unknown options to suggest to the user what they need to do in order to pass options through to PETSc.

@GregVernon
Copy link
Author

GregVernon commented Jan 16, 2023

@dham / @wence- -- I'm just coming around back to this, grad school got a bit busy. I've made a few modifications based on conversations above, namely I believe it's building documentation (placed link right under the demo for Gmsh). I've also setup Exodus only to build if the user doesn't use PETSc minimal and specifies --with-exodusii in their firedrake-install command.

@dham
Copy link
Member

dham commented Jan 17, 2023

@dham / @wence- -- I'm just coming around back to this, grad school got a bit busy. I've made a few modifications based on conversations above, namely I believe it's building documentation (placed link right under the demo for Gmsh). I've also setup Exodus only to build if the user doesn't use PETSc minimal and specifies --with-exodusii in their firedrake-install command.

Thanks @GregVernon. This is very nearly there. Please add something to the top of the demo documentation that says that it's necessary to pass --install-exodus to firedrake-install or firedrake-update. Otherwise users are likely to be confused as you were (it would also be good to add this comment to the manual).

We also need --install-exodus to apply even if --minimal-petsc is specified. This is because the CI testers run with minimal PETSc and will need exodus installed when they test your demo (all demos are run by the CI to ensure they still work). Finally, you need to modify .github/workflows/build.yml to add the --install-exodus option to the firedrake-install line.

Cheers,

David

@GregVernon GregVernon force-pushed the cubit/demos branch 5 times, most recently from 5f4a704 to 55ef221 Compare January 22, 2023 02:13
@GregVernon
Copy link
Author

GregVernon commented Jan 22, 2023

@dham

Thanks @GregVernon. This is very nearly there. Please add something to the top of the demo documentation that says that it's necessary to pass --install-exodus to firedrake-install or firedrake-update. Otherwise users are likely to be confused as you were (it would also be good to add this comment to the manual).

Ok, I've added this to a couple spots.

  1. The demo
    image

  2. The "Constructing Meshes" documentation
    image

We also need --install-exodus to apply even if --minimal-petsc is specified. This is because the CI testers run with minimal PETSc and will need exodus installed when they test your demo (all demos are run by the CI to ensure they still work). Finally, you need to modify .github/workflows/build.yml to add the --install-exodus option to the firedrake-install line.

Done! I'm not sure how to test this, as I can't seem to run the CI testers on my fork.

@GregVernon
Copy link
Author

@dham -- any chance we can merge this today?

@connorjward
Copy link
Contributor

@dham -- any chance we can merge this today?

Added to today's agenda

@JDBetteridge
Copy link
Member

Do the binary .e files need to be added or are they generated from the .jou files?

Copy link
Member

@dham dham left a comment

Choose a reason for hiding this comment

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

The reasons this is failing tests are:

  1. Whitespace issues in firedrake-install. Please run make lint in firedrake/src/firedrake and do what it says.
  2. build.yml has a misspelt option.

.github/workflows/build.yml Outdated Show resolved Hide resolved
@GregVernon GregVernon force-pushed the cubit/demos branch 2 times, most recently from d027d1d to 035ab82 Compare January 25, 2023 17:06
@GregVernon
Copy link
Author

@JDBetteridge said:

Do the binary .e files need to be added or are they generated from the .jou files?

I did add the .e files (demos/immersed_fem_cubit/immersed_domain_*.e) so that a Cubit license and/or instance isn't required for a user to test functionality -- especially where the "user" is CI. The journal files do recreate these files.

@GregVernon
Copy link
Author

@dham / @JDBetteridge -- any chance that you can approve me to run workflows? I certainly don't want merge privileges, but being able to run workflows immediately rather than waiting a week or more would be very helpful for me being able to finish this.

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