search

jonaslb / psiesta

Python bindings for "Siesta as a subroutine" DFT
https://github.com/jonaslb/psiesta
GNU General Public License v3.0
2 stars 2 forks source link

Links to your branch #3

Closed zerothi closed 4 years ago

zerothi commented 4 years ago

Hi Jonas, could you update your links in your README, they point to github. But you refer to gitlab (where you have multiple branches).

I would like to help getting this in siesta-master. So if you could pin-point the branch?

jonaslb commented 4 years ago

Hey, I can see the link wasn't properly inserted in the markdown document (without the https:// it becomes a relative github link, apparently). It is here: https://gitlab.com/jonaslb/siesta

The jolubmaster branch contains a merge of two (three) working branches with relevant commits in each:

They are based on siesta master from a couple of months ago. For me, the first fix was entirely necessary, the second one only needed for TS.S.Save runs. Note if you check the diffs in gitlab, disabling 'show whitespace changes' can help. My editor automatically removed superfluous whitespace at the time.

I initially created a pull request for the gitignore-branch, and decided to wait with the other branches until that was accepted. It took (much) longer than anticipated so I kind of got away from it. If you think they are ready I can just create a couple of PRs (maybe squash the two commits related to reopening stdout)?

If I could whish one thing from Siesta it would be that it was easier to build and link :) I hear a lot of good about Meson, so I thought about using it for this project along with wrappers for Siesta and its libraries. But perhaps it'd be better if this was in Siesta itself? I think it could make things at least in this project dramatically simpler.

zerothi commented 4 years ago

I agree the build-system needs a revamp for Siesta. Problem is, adding a build-system requires maintenance. We had autotools but it didn't get maintained.
There is work planned for re-adding the autotools method in another branch. Lets see if it comes in. I also have had a look @ meson and should try and get it to work for fdict, flook and ncdf. But I haven't had the time yet... So we are well aware of the current situation. But 1) the current build-system should be retained. 2) the meson should be an optional build-system 3) we are currently waiting for a large merge for a 2nd branch that will change the build-system a bit. Once this is in, I think we can have a discussion about it. I'll try to remember to open an issue once psml is in.

I'll have a look at your branches. Thanks!

zerothi commented 4 years ago

@jonaslb I have had a look.

Could you please make a PR for:

As for the onlys, I see you want this since you want siesta_forces to return and not do bye, could you confirm this?

jonaslb commented 4 years ago

Ok, I will look at rebasing, squashing and creating a PR for the reopening of stdout :)

Regarding onlys, that is true. There was issues with the timer calls, in that the timer can be started in one method and stopped in another, creating a mess when doing a premeture return. But I believe it is done correctly now (at least, not crashing). There was also some possibly unused code about "S only" which sounded a lot like "onlyS", so I considered removing it, but in the end didn't because I wasn't sure. The problem with bye is that it stops the python process, while return gives back control. Since one often wants to do several onlys'es in one go, it was quite annoying to see the process forcefully end ;)

zerothi commented 4 years ago

Agreed, I think your logic is good. Ok, this should definitely go in!

jonaslb commented 4 years ago

Siesta merge requests !15 & !16 have been opened. Let's continue the talk there. I will update the readme here when they are merged :)

zerothi commented 4 years ago

Thanks, lets do that!

jonaslb commented 4 years ago

The changes are merged, I've updated the readme to mention the need for a recent Siesta master version.