Skip to content

Conversation

@CKehl
Copy link
Contributor

@CKehl CKehl commented Feb 9, 2021

Based on the performance branches (#862 , #884 , #957 , #958 and #959 ) and in preparation for merging #958 , we estanlish a kernel hierarchy, with a base kernel (basekernel.py) from with ParticleSet-specific derivatives and None-fieldset derivatives can derive from. The latter one is just meant for larger extension purposes, as a testbed-draft.

This issue is linked with #862 (initial basekernel.py), #958 (target ParticleSet to be incorporated after changes), #866 (proper object-oriented programming) and #618 (earlier starting point).

…nges in kernel.py (master) with kernelbase.py (sorted_lists_pset)
@CKehl CKehl self-assigned this Feb 9, 2021
CKehl and others added 15 commits February 9, 2021 18:09
…soa.py that are already defined in basekernel.py
…e issues emerging from running the tests. tested and working.
@CKehl
Copy link
Contributor Author

CKehl commented Feb 10, 2021

ready for review. If you're interested what I did: the commits are structured in a step-by-step way to better understand the changes.

Copy link
Member

@erikvansebille erikvansebille left a comment

Choose a reason for hiding this comment

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

Good cleanup of the code. A few minor comments

  • there seem to be a few stray commented lines (from debugging?) left in the code. Remove these for a cleaner code?
  • We previously moved all the particleset*.py files to a new subdirectory. Should we do the same for these kernel*.py files, for consistency?
  • Not sure why the CI on Windows is failing?

@CKehl
Copy link
Contributor Author

CKehl commented Feb 11, 2021

Good cleanup of the code. A few minor comments

* [ ]  there seem to be a few stray commented lines (from debugging?) left in the code. Remove these for a cleaner code?

* [ ]  We previously moved all the particleset*.py files to a new subdirectory. Should we do the same for these kernel*.py files, for consistency?

* [ ]  Not sure why the CI on Windows is failing?

(a) first trying to trace and eradicate Win32 compile errors
(b) well, moving the files is difficult. We cannot put e.g. the kernelsoa.py in kernels/, because this will result in circular imports (i.e. import errors) - that's because because RK4 would import BaseKernel and KernelSOA imports RK4. I'd rather have suggested to create folders such as soa/, aos/ etc., and put particlesets, collections, kernels and partilcefiles (to come) of one structure in the same folder - thus getting rid of the particlesets folder. for now, I leave it as is, see if all runs smoothly.
(c) comment removal - thy will be done.

@CKehl CKehl requested a review from erikvansebille February 11, 2021 16:55
Copy link
Member

@erikvansebille erikvansebille left a comment

Choose a reason for hiding this comment

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

I had some more time to carefully look through the changes, and have two slightly larger comments (sorry for not raising these earlier); as well as still a commented line of old code here and there that could go?

Copy link
Member

@erikvansebille erikvansebille left a comment

Choose a reason for hiding this comment

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

Happy with all the changes if the last two debug lines are removed. Feel free to merge after that

@CKehl CKehl merged commit 2ee4309 into master Feb 12, 2021
@CKehl CKehl deleted the generalize_kernels branch February 12, 2021 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants