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

Low-level API Poisson equation #71

Merged
merged 66 commits into from
Mar 17, 2021
Merged

Low-level API Poisson equation #71

merged 66 commits into from
Mar 17, 2021

Conversation

amartinhuertas
Copy link
Member

Hi @fverdugo, @santiagobadia,

after some final touches (see issue #60 for more details), I think the Low-Level tutorial is ready to merge!

The only thing which is missing is the following observation:

https://github.com/gridap/Tutorials/blob/dev-tutorials/src/poisson_dev_fe.jl#L368

for which I do not have an answer yet. We can leave it there, remove it, or try to find the reason, answer and solve it if necessary.

What do you think?

santiagobadia and others added 30 commits August 12, 2020 16:07
* Added that HEX_AXIS will create n-cubes
* Fixed minor typo
0.15

Documentation comments still pending to be adapted.
…p#master

* Updated Manifest, among other, to point to Gridap#master
@amartinhuertas
Copy link
Member Author

Hi @fverdugo ... I went over all your comments in regards to this PR. See #71 (comment) for more details.

I had the following issues:

  • I tried to put consecutive lines which I want in different code cells by putting a blank line among them, but it did not work. Do u know how to do this?
  • The usage of print_op_tree causes an error in the CI system. https://github.com/gridap/Tutorials/runs/2084255358?check_suite_focus=true#step:6:1 Have you ever seen this error?
  • I could not test whether Debugger works in JNs. My local installation is broken, and I do not want to fix it now because I am lecturing with JNs. Can you try this and comment at the begnining of the tutorial if this possibility exists?
  • The other points which are under the pending header are points which I did not actually know what you pretended I do with them.

@fverdugo
Copy link
Member

Thanks! I'll take a look.

@fverdugo
Copy link
Member

I tried to put consecutive lines which I want in different code cells by putting a blank line among them, but it did not work. Do u know how to do this?

Simply by adding an empty comment line between them. I.e.,

DomainStyle(Qₕ) == ReferenceDomain()
#
DomainStyle(Qₕ) == PhysicalDomain()

I could not test whether Debugger works in JNs. My local installation is broken, and I do not want to fix it now because I am lecturing with JNs. Can you try this and comment at the begnining of the tutorial if this possibility exists?

To simplify things (for us and for the reader), I would write the tutorial assuming that the reader will do it by executing the cells in the notebook (just like the other tutorials). If you need to show intermediate results, you can split the cells as commented above. I would recommend to use a debugger just as an optional step.

The usage of print_op_tree causes an error in the CI system. https://github.com/gridap/Tutorials/runs/2084255358?check_suite_focus=true#step:6:1 Have you ever seen this error?

I have never seen this error, but by looking at the implementation of print_op_tree in Gridap it seems that there is a bug:

https://github.com/gridap/Gridap.jl/blob/3e3b006b5046b628449c0053af4e5c82c5063111/src/Arrays/PrintOpTrees.jl#L34

It should be stdout instead of stdin, right?

The other points which are under the pending header are points which I did not actually know what you pretended I do with them.

The issue about spurious memory allocation still has to be further investigated...

@fverdugo
Copy link
Member

Hi @amartinhuertas! please find my replay to your last questions above ☝️

@fverdugo
Copy link
Member

Regarding the bug in print_op_tree gridap/Gridap.jl#563

trying to address without success
Rewrote the first passages taking into account the following remark

"
To simplify things (for us and for the reader), I would write the tutorial assuming that
the reader will do it by executing the cells in the notebook (just like the other tutorials).
"
corresponding to Gridap.jl git repo #master branch so that we
have access to the following bug-fix:

gridap/Gridap.jl#563
@amartinhuertas
Copy link
Member Author

Simply by adding an empty comment line between them. I.e.,

Ok, I do not know why, but I though this was a dirty hack, and I was expecting a more sophisticated solution (e.g., an special markdown tag). I already followed this approach in 83f986d.

It should be stdout instead of stdin, right?

Yes. it should be stdout. I am now leveraging your bug-fix in this branch. Let us see whether this solves the problem.

The issue about spurious memory allocation still has to be further investigated...

Yes, but apart from this, there are still two additional comments without solution yet.

@amartinhuertas
Copy link
Member Author

Regarding the bug in print_op_tree gridap/Gridap.jl#563

This solves the issue in the tutorials CI ! Great!

@fverdugo
Copy link
Member

export FEBasis, BasisStyle, TestBasis and TrialBasis from FESpaces (but not from Gridap)

The definition of FEBasis will change in Gridap v0.16. FEBasis will be an abstract type, with SingleFieldFEBasis and MultiFieldFEBasis specializations. I expect to register Gridap v0.16 during this week. They will be exported from the corresponding modules.

The actual purpose of Broadcasting is allowing to write lazy_map(Broadcasting(*),a,b) instead of lazy_map((ai,bi)->ai.*bi,a,b). The main reason is syntactic sugar (even though there are a number of dispatches specialized for Broadcasting for performance reasons.)

This is just a minor comment.

@amartinhuertas
Copy link
Member Author

Ok ... so the pull request can be accepted, right?

@fverdugo
Copy link
Member

Ok ... so the pull request can be accepted, right?

By looking at the Manifest, it seems that this PR relies on an unregistered version of Gridap. I'd wait until we release Gridap 0.15.3 with the patch related to the print_op_tree and change the Manifest here accordingly.

@amartinhuertas
Copy link
Member Author

amartinhuertas commented Mar 16, 2021

By looking at the Manifest, it seems that this PR relies on an unregistered version of Gridap.

Yeah, true. I should remove the Manifest.toml from the github repo.

I'd wait until we release Gridap 0.15.3 with the patch related to the print_op_tree and change the Manifest here accordingly.

Ok. Let us wait until then. Are we waiting for a particular event in order to release Gridap 0.15.3? If not, I can help to prepare this new release.

@fverdugo
Copy link
Member

Ok. Let us wait until then. Are we waiting for a particular event in order to release Gridap 0.15.3? If not, I can help to prepare this new release.

Perfect if you can release this!

The only part missing is to add a couple of entries in the NEWS.md related with the last PRs, move to 0.15.3 in the Project.toml, check that all tests pass and release. See also https://github.com/gridap/Gridap.jl/wiki/How-to-register-a-julia-package#specific-steps-to-registering-gridap

@amartinhuertas
Copy link
Member Author

The only part missing is to add a couple of entries in the NEWS.md related with the last PRs

Looking at the PRs accepted since the last release (0.15.2), I see (ordered by PR id, not chronologically): #553, #555, #556, #559, #563, #564. If this is the case, we should add 6 entries to the NEWS.md file (not actually a couple), right? On the other hand ... are all these minor release patches?

@fverdugo
Copy link
Member

fverdugo commented Mar 16, 2021

Yes, sorry. We usually only include the most relevant changes in NEWS.md, it is not needed to include all PRs.

In this case, I would mention that 1) we have fixed a bug in print_op_tree and 2) that the cell map in meshes of linear simplices is now an array of affine maps, which allows one to compute laplacians of the shape functions among other things.

@amartinhuertas
Copy link
Member Author

Ok, thanks! PR requested in gridap/Gridap.jl#566

@amartinhuertas
Copy link
Member Author

@fverdugo ... all tests passed with Gridap 0.15.3 (new release). I think the PR is ready to merge.

Copy link
Member

@fverdugo fverdugo left a comment

Choose a reason for hiding this comment

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

A couple of minor comments and we are ready to go!

@@ -0,0 +1,17 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can add this file to .gitignore

Copy link
Member Author

Choose a reason for hiding this comment

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

It was already in .gitignore. Possible added accidentally into the repository before was in gitignore. I have removed the file.

@@ -15,7 +16,6 @@ Printf = "de0858da-6303-5e67-8744-51eddeeeb8d7"
Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"

[compat]
Gridap = "0.15"
Copy link
Member

Choose a reason for hiding this comment

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

Please, do not remove Gridap from the compat section. Use Gridap 0.15.3 since this is the version that has the bugfix we need.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, Added. But I was not aware that I had removed it ... perhaps it was the package manager automatically or something like that?

@fverdugo
Copy link
Member

Hi @amartinhuertas. Thanks for the changes.

Just a question, what is dev/Gridap for? Do we need it?

@amartinhuertas
Copy link
Member Author

Just a question, what is dev/Gridap for? Do we need it?

I dont think so. Removed. You have 100 eyes!

@fverdugo fverdugo merged commit 388a329 into master Mar 17, 2021
@fverdugo fverdugo deleted the dev-tutorials branch March 17, 2021 07:37
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.

7 participants