General Tasks

[iakovts]

Hello again @flaport,

I 've been reading through the code, and I have suggestions (that I could also possibly implement):

1. Automate tests through github Actions. (I ve actually already implemented a PoC on my fork - will open a PR). In the future Actions can also be used for publishing docs and the package itself on pypi.

2. Some (minor?) code refactoring, hopefully without having to change the current API at all. An example of what I have in mind is changing this to a class factory and generally some clarity changes.

3. [edit] I would also suggest using another branch for active development/merges were changes are frequent (dev/staging?) and using master as a definitive branch for releases.

Looking forward to your thoughts on the above and more :)

[flaport]

Hi @iakovts,

Those are some great ideas!

1. I've always wanted to add some github actions to one of my repos. So far I haven't really taken the time to get to know it very well (I'm personally more comfortable with gitlab CI) so having someone do the first steps would be great! Thanks 😀

2. All code cleanup is welcome! Just keep in mind that I also want to keep the backend code accessible to scientists/researchers that don't always have a computer science background or are not always as comfortable with python as you or me. Sometimes that means having some code duplication for readability and ease of access. But feel free to make a PR for this, I'm pretty sure there is quite a bit that can be improved 😉

3. Yep. Sounds good. Alternatively, we could also just use feature branches which we merge into master. But for now I created a dev branch here.

[iakovts]

Hey @flaport,

1. I'd be happy to keep working on Actions (including what you mentioned on Test workflows PoC #20), even though tbh it was my first time working with Actions too haha (only had gitlab-ci experience). As for the building a wheel part (+ publishing it on pypi I assume), I might need some help there since I've only done that 1-2 times before in a different environment. (P.S. I've used act for testing the Actions locally before moving to github and it seems to work great).
2. Yeah, what I had in mind would not change the user's interaction with the package at all - just some clean-up / refactoring for developers. I'll try to provide an example when I have the time (probably by next week). Tbh it'd be great if more tests were available, since these changes might lead to side-effects. I might be able to start working on tests too, but some help would be greatly appreciated since I am not familiar with the project yet from a user standpoint as I haven't really started using it yet apart from some trying out some examples etc. - I suggest tracking refactoring and test changes on different issues.
3. Great! I'll use that.

[flaport]

Hey @iakovts,

1. I'm currently pretty short on time, so I'll merge the PR for now and we can figure out how to auto build and publish wheels on a later time. This act tool seems cool, I'll definitely check it out!
2. I know, I should add tests, but same as above... I'm lacking the time to work on them. I'll see what I can do in the next few weeks though...

[iakovts]

Hey @flaport,

So I've started working on the refactoring of boundaries, and as an example I've made some changes to periodic boundaries. These changes are purely "cosmetic", just made to avoid some repetitions of similar classes etc. I would also like to avoid setting self.__class__ = ... there as I believe it's too implicit, but that would be probably require some major changes (plus I am still not super familiar with the whole package so it could be unavoidable?).

Also, in a completely unrelated note, is self.loc missing a slice(None) here ? I was probably going to start working on it too and I noticed it.

[flaport]

thanks @iakovts, this looks good!

I think there should be a way around setting self.__class__, which I must admit I also don't like very much. I can have a look after merging this.

For numpy arrays slice(None) at the end of the tuple is equivalent with not adding it at all (e.g. think of x[:, :] vs x[:] for a 2D array). However, it's probably better to be more explicit here as well and add it for completeness.

[iakovts]

Hey @flaport, sorry for the late response.
Oops I didn't realize that for the numpy slices (even though it's something I'm actually using a lot - just didn't put 2 and 2 together till now haha).
Ok, I ll keep working on the boundaries and open a PR when ready!