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

Renovate Dimension object #2953

Merged
merged 17 commits into from
Nov 22, 2023
Merged

Renovate Dimension object #2953

merged 17 commits into from
Nov 22, 2023

Conversation

TiborGY
Copy link
Contributor

@TiborGY TiborGY commented May 6, 2023

Description

This PR aims to improve the Dimension object in various ways, by adding machine-readable (for eg. VSCode) docstrings, adding const to arguments, replacing int with size_t to match std::vector::operator[](std::size_t) and cautiously deprecating member functions that deal with pointers.

Not only are pointers more error-prone, they are currently involved in some strange interaction between Dimension and Matrix objects, resulting in awkward roundabout initialization/construction that I hope to untangle before Psi4 1.11? (1.11, or maybe 1.12?).
Please let me know if you feel the three deprecations in this PR would be too disruptive or otherwise undesirable, this PR is very much a "request for comments".

User API & Changelog headlines

  • API change: The custom assignment operator for Dimension objects (Dimension& operator=(const int*)) is being deprecated. Unless someone speaks up, 1.10 may be the last release to have it.
  • API change: Cast-to-pointer operators for Dimension objects (operator int*() and operator const int*() const) are being deprecated. Unless someone speaks up, 1.10 may be the last release to have them.
  • Minor API change: Several constructors and member functions of Dimension are now using size_t instead of int for indexing:
    Dimension::Dimension(int, const std::string&) is now Dimension::Dimension(size_t, const std::string&)
    void Dimension::init(int, const std::string&) is now void Dimension::init(size_t, const std::string&)
    int Dimension::n() const is now size_t Dimension::n() const
    int& Dimension::operator[](int) is now int& Dimension::operator[](size_t)
    const int& Dimension::operator[](int) const is now const int& Dimension::operator[](size_t) const
    const int& Dimension::get(int) const is now const int& Dimension::get(size_t) const
    void Dimension::set(int, int) is now void Dimension::set(size_t, int)

Dev notes & details

  • Machine-readable docstrings have been added to dimension.h to improve suggestions offered by IDEs like VSCode
  • size_t is now used instead of int when dealing with array indexing. Python bindings have been updated to reflect the change in constructor arguments.
  • Local variables have been made const where possible
  • Deprecation notices have been added to Dimension& operator=(int*), operator int*() and operator const int*() const

Checklist

  • No new features
  • Tests run by the CI are passing

Status

  • Ready for review
  • Ready for merge

@TiborGY TiborGY marked this pull request as ready for review July 8, 2023 19:09
@fevangelista
Copy link
Contributor

fevangelista commented Nov 13, 2023

I had to write some code recently using the Dimension object and I was thinking that it would be useful to support initialization from a list, for example:

Dimension orbs({3,0,1,1})

This is particularly useful when working with Slices to avoid doing this:

        auto start_dim = psi::Dimension(1);
        start_dim[0] = start;

I think it might already be supported on the python side. Another useful thing would be to have direct access to the underlying std::vector object, or const iterators for it. That way, one could use Dimensions in loops more naturally.

I was wondering, since you are modifying the class, would you be able to implement these changes as well?

@TiborGY
Copy link
Contributor Author

TiborGY commented Nov 13, 2023

I was wondering, since you are modifying the class, would you be able to implement these changes as well?

I could look into it, but I have other priorities for the next couple of months. I am also yet to receive any feedback/reviews on the changes pushed so far, so I would prefer to have the commits of this PR to be about refactoring only.
I think adding new features would be better done in a separate PR, after this is merged or rejected.

Copy link
Member

@loriab loriab left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, though I'm not a big user of Dimension and will defer to others. This would be a good time to get the deprecation warnings in, so the fns can be removed by 1.10. Are there any Python bindings that hit the deprecated fns? I think we could inject runtime depr warnings in python_helpers.py. The downside of the c-side warnings is that one only sees them when compiling.

psi4/src/psi4/libmints/dimension.h Show resolved Hide resolved
@TiborGY
Copy link
Contributor Author

TiborGY commented Nov 14, 2023

Are there any Python bindings that hit the deprecated fns?

I don't think so? They are not used in py::class_<Dimension> in export_mints.cc, if there are other places to look for Python bindings let me know and I will check.

This would be a good time to get the deprecation warnings in, so the fns can be removed by 1.10

Perhaps, but I delayed it because

  1. I was not sure that this would get thorough reviews before 1.9 is released. This is something that could disrupt people's codes/plugins that rely on it. I do not know how widely they are used, thus I want to give people time to stumble into the deprecation warning and yell before it is too late.
  2. These functions are not exactly unused, even in Psi4 itself. It will not be a trivial task to rip them out, and I doubt that I will have enough free time to complete it before May 2024.

@loriab
Copy link
Member

loriab commented Nov 14, 2023

Are there any Python bindings that hit the deprecated fns?

I don't think so? They are not used in py::class_ in export_mints.cc, if there are other places to look for Python bindings let me know and I will check.

Looks like export_mints.cc only, so you've got that covered, thanks.

This would be a good time to get the deprecation warnings in, so the fns can be removed by 1.10

Perhaps, but I delayed it because

  1. I was not sure that this would get thorough reviews before 1.9 is released. This is something that could disrupt people's codes/plugins that rely on it. I do not know how widely they are used, thus I want to give people time to stumble into the deprecation warning and yell before it is too late.
  2. These functions are not exactly unused, even in Psi4 itself. It will not be a trivial task to rip them out, and I doubt that I will have enough free time to complete it before May 2024.

Sounds fair. Two cycles may be good for the deprecation warning, as plugin devs like clear warnings.

@loriab loriab added this to the Psi4 1.9 milestone Nov 14, 2023
Copy link
Contributor

@fevangelista fevangelista left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I requested some changes, some of which should be propagated to both the .h and .cc files.

psi4/src/psi4/libmints/dimension.cc Outdated Show resolved Hide resolved
psi4/src/psi4/libmints/dimension.cc Outdated Show resolved Hide resolved
psi4/src/psi4/libmints/dimension.cc Outdated Show resolved Hide resolved
psi4/src/psi4/libmints/dimension.cc Outdated Show resolved Hide resolved
psi4/src/psi4/libmints/dimension.h Outdated Show resolved Hide resolved
psi4/src/psi4/libmints/dimension.h Outdated Show resolved Hide resolved
psi4/src/psi4/libmints/dimension.h Outdated Show resolved Hide resolved
@loriab loriab added this pull request to the merge queue Nov 22, 2023
Merged via the queue into psi4:master with commit 991aa25 Nov 22, 2023
5 checks passed
@TiborGY TiborGY deleted the dim_rework_b branch February 6, 2024 14:47
@TiborGY TiborGY mentioned this pull request Apr 26, 2024
20 tasks
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.

None yet

3 participants