-
Notifications
You must be signed in to change notification settings - Fork 2
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
PROM DG advection #18
Conversation
@@ -23,7 +23,7 @@ RUN sudo git clone https://github.com/LLNL/libROM.git | |||
WORKDIR ./libROM | |||
RUN sudo git pull | |||
# pylibROM is currently based on a specific commit of the libROM. | |||
RUN sudo git checkout mfem_fix | |||
RUN sudo git checkout 0809d7d09dc24f0963c38fc8c0a2649948142ba0 |
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.
It looks like this change was actually part of #17, but should this be changed to be the head of the main branch of libROM? This commit SHA seems to be outdated since a few PRs have been merged in libROM. Maybe this should be changed in a separate PR to make sure the two repos stay in sync.
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.
I guess we are using a static commit for libROM to keep pylibROM stable and avoid any unpredicted breakdowns due to the actively changing C++ libROM. From time to time I saw Kevin change the SHA, after some necessary changes were made in C++ libROM. @dreamer2368 , may I ask if it is accurate?
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.
This is correct. We'd like to update SHA occasionally, but at the same time specify SHA explicitly which can be information for debugging. I think mfem_fix
branch is now merged to main branch of the libROM, so it's okay to update this SHA now.
@siuwuncheung Do you have any ideas why |
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.
Looks good to me and seems to reproduce the C++ example.
Thanks for the review and asking the question, @ckendrick. I had not checked that what exactly causes it to be slow, but I also suspect it is something due to the bindings. I saw in Kevin's Poisson PROM example that it sets a maximum of 5 snapshots used in |
Was it on your local macbook? The performance can vary from optimum, especially because the docker image is based on different architecture.
I don't exactly remember how the maximum number was set there. I should check again, but it should be simply a translation from c++ example. |
It was on my M1 chip macbook. Can you try to run the parametric predictive example on your machine and see how much time the merge phase takes?
|
Running on quartz with singularity (docker container) returned the same result. The merging time was about 120 seconds, which is shorter than 2000 seconds, but still longer than expected for 3 samples. Will run without container as well. update If not, we should narrow down what python-wrapped function incurs this overhead cost, and investigate if it's possible to reduce the overhead. |
It takes ~60 seconds in C++ libROM on Quartz. I guess it's safe to say that some Python-wrapped function incurs the extra cost, but the syntax is correct in this PR? It seems to me that it is just we've not had a case with that many snapshots to reveal that |
Update: |
BasisGenerator
Sidenotes:
libROM.Matrix
tomfem.DenseMatrix
does not need tranposeloadSamples
seems much slower in Python (build with Docker) than C++. In the parametric case, it takes 2000 seconds.Update:
On M1 chip macbook, merge phase in pylibROM (built with C++ docker container for ARM) in 62 seconds, while the C++ takes 64 seconds. The overhead is probably due to using Docker built from a different architecture.