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

Add deisa plugin #473

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
Open

Add deisa plugin #473

wants to merge 40 commits into from

Conversation

benoitmartin88
Copy link
Member

@benoitmartin88 benoitmartin88 commented Oct 2, 2024

Short description of the MR

Add Deisa plugin to enable in-situ analysis.

List of things to check before making a PR

Before merging your code, please check the following:

  • you have added a line describing your changes to the Changelog;
  • you have added unit tests for any new or improved feature;
  • In case you updated dependencies, you have checked pdi/docs/CheckList.md
  • you have checked your code format:
    • you have checked that you respect all conventions specified in CONTRIBUTING.md;
    • you have checked that the indentation and formatting conforms to the .clang-format;
    • you have documented with doxygen any new or changed function / class;
  • you have correctly updated the copyright headers:
    • your institution is in the copyright header of every file you (substantially) modified;
    • you have checked that the end-year of the copyright there is the current one;
  • you have updated the AUTHORS file:
    • you have added yourself to the AUTHORS file;
    • if this is a new contribution, you have added it to the AUTHORS file;
  • you have added everything to the user documentation:
    • any new CMake configuration option;
    • any change in the yaml config;
    • any change to the public or plugin API;
    • any other new or changed user-facing feature;
    • any change to the dependencies;
  • you have correctly linked your MR to one or more issues:
    • your MR solves an identified issue;
    • your commit contain the Fix #issue keyword to autoclose the issue when merged.

* update embedded pybind11 to 2.11.1 to fix recent python version problems
@benoitmartin88 benoitmartin88 added this to the v1.8.0 milestone Oct 2, 2024
@benoitmartin88 benoitmartin88 self-assigned this Oct 2, 2024
@benoitmartin88 benoitmartin88 linked an issue Oct 2, 2024 that may be closed by this pull request
@benoitmartin88 benoitmartin88 marked this pull request as draft October 2, 2024 15:46
@benoitmartin88 benoitmartin88 changed the title [WIP] add deisa plugin Add deisa plugin Oct 2, 2024
@benoitmartin88
Copy link
Member Author

For the sake of merging this feature quickly, I will open a new issue for the tests.
This plugin has been used with success, so I feel confident that it works well.

@benoitmartin88 benoitmartin88 marked this pull request as ready for review October 3, 2024 15:29
.gitignore Outdated Show resolved Hide resolved
AUTHORS Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
pdi/docs/Source_installation.md Outdated Show resolved Hide resolved
pdi/docs/Plugins.md Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
@pdidev pdidev deleted a comment from jbigot Oct 11, 2024
Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

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

A few propositions

CMakeLists.txt Outdated Show resolved Hide resolved
pdi/docs/Source_installation.md Outdated Show resolved Hide resolved
pdi/src/python/tools.cxx Outdated Show resolved Hide resolved
plugins/deisa/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/deisa/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
Yushan-Wang
Yushan-Wang previously approved these changes Oct 22, 2024
Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

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

Just a few remaining very minor remarks

example/example.c Outdated Show resolved Hide resolved
example/example.c Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
benoitmartin88 and others added 3 commits November 4, 2024 09:37
Co-authored-by: Julien Bigot <jbigot@users.noreply.github.com>
Co-authored-by: Julien Bigot <jbigot@users.noreply.github.com>
jbigot
jbigot previously approved these changes Nov 5, 2024
Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

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

I believe this is the last batch of comments. This ended up being a much bigger cleanup than I anticipated, but I believe this will be good in the long term

example/README.md Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
Co-authored-by: Julien Bigot <jbigot@users.noreply.github.com>
benoitmartin88 and others added 2 commits November 5, 2024 16:27
Co-authored-by: Julien Bigot <jbigot@users.noreply.github.com>
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
jbigot
jbigot previously approved these changes Nov 8, 2024
Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

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

Sorry, I missed a point. But apart from these 3 small changes that I forgot, I'd say it's good to ship.

Great cleanup work, I really believe the plugin is now in a much better situation and I'm now confident for its future!

plugins/deisa/deisa.cxx Show resolved Hide resolved
plugins/deisa/deisa.cxx Show resolved Hide resolved
plugins/deisa/deisa.cxx Outdated Show resolved Hide resolved
Copy link
Member

@Yushan-Wang Yushan-Wang left a comment

Choose a reason for hiding this comment

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

Great!

Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

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

Congrats, we're there now 🎉

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.

Add Deisa plugin
3 participants