Rename zero_out_ghosts() to zero_out_ghost_values() #11461

peterrum · 2021-01-04T11:52:25Z

The asymmetric of update_ghost_values() and zero_out_ghosts() only leads to confusion. I would suggest to also rename has_ghost_elements() to has_ghost_values().

FYI @mschreter

kronbichler · 2021-01-04T12:08:36Z

include/deal.II/lac/la_parallel_block_vector.h

       */
-      void
+      DEAL_II_DEPRECATED void


I agree with the general PR and with @deprecated in the doxygen comment, but I am very unhappy about adding the DEAL_II_DEPRECATED marker for now.

We've had this discussion before, but I want to once again point out the pain this causes in downstream projects. This change raises deprecation warnings from day one without the users having any time to use a different interface, so we force all downstream projects without very tight control on the deal.II version to add #ifdef markers. For more commonly used functions like the present one, I suggest to have one release with both interfaces (and the preferred one shown in tutorials), one release with the deprecation warning, and then finally remove the old interface two releases ahead.

I want to contrast this to what I consider a gentle deprecation process, e.g. the MatrixFree class where we now deprecated n_macro_cells but we had at least one previous release with what we suggest as alternative. This still forces users to eventually upgrade and gives us a chance to eventually fix an interface we do not like, but without being explicitly rude against our users.

I agree with the general PR and with @deprecated in the doxygen comment, but I am very unhappy about adding the DEAL_II_DEPRECATED marker for now.

I have removed the marker.

bangerth · 2021-01-05T03:59:52Z

include/deal.II/lac/la_parallel_block_vector.h

+
+      /**
+       * This method zeros the entries on ghost dofs, but does not touch
+       * locally owned DoFs.


I would prefer if we talked about "ghost entries" of vectors, rather than DoFs. They are often the same, but not necessarily so.

bangerth · 2021-01-05T04:00:48Z

include/deal.II/lac/la_parallel_block_vector.h

+       * locally owned DoFs.
+       *
+       * After calling this method, read access to ghost elements of the
+       * vector is forbidden and an exception is thrown. Only write access to


An exception is thrown when? I think what you say is that an exception is thrown if one tries to read from these values, but that's not clear from this one sentence (though the next clarifies it).

It's an interesting concept, though. Do any other functions impose this kind of restriction? When is the restriction lifted?

bangerth · 2021-01-05T04:02:23Z

include/deal.II/lac/la_parallel_block_vector.h

@@ -348,10 +348,24 @@ namespace LinearAlgebra
       * After calling this method, read access to ghost elements of the
       * vector is forbidden and an exception is thrown. Only write access to
       * ghost elements is allowed in this state.
+       *
+       * @deprecated Use zero_out_ghost_values() instead.
       */
      void
      zero_out_ghosts() const;


Can you mark up this function with DEAL_II_DEPRECATED?

Ah, I'm only now reading @kronbichler 's comment.

How do we track these things? If we don't DEAL_II_DEPRECATE these functions right away, then we should at least have an issue somewhere that collects all of those symbols we'd like to deprecate after the next release...

Yes, also contrib/utilities/print_deprecated.py always searches for DEAL_II_DEPRECATED.
I don't think we really came to a conclusion regarding our policy to not deprecate immediately...
I still don't see a problem if codes that use upstream just always disable deprecation warnings if they don't want to deal with them.

I am probably biased on this topic because I see the cost of taking care of these warnings in maybe half a dozen projects and I work with upstream. But it is not just upstream (how large is the percentage of users on master vs a release, by the way?). I think many projects gravitate around two subsequent releases of deal.II. For me it is the latest release and master, but it does not really change much from a user perspective if you work with say the 9.2 and 9.1 releases. If you do not want warnings that came up in 9.2 and where 9.1 did not yet provide an alternative, you need all users of a project using deal.II to update at the same time, unless you ifdef by version. For small functions, ifdef is fine, but for more widely used interfaces it is a pain. For single-person projects an immediate deprecation warning is also no problem, because you simply decide to fix the outdated interface that you're told by the warning. But for multi-person projects you cannot or do not want to control the exact deal.II version. So to me the important part is that we have one release with the new alternative and without the warning. Note that I am not opposed to eventually deprecate, but a distance of two releases is much easier to handle, as I see no problem to force users not being more than one release behind whatever project XY sees as the most recent deal.II version.

To me, the question is what we think makes best value out of the deprecation warnings. If we are too aggressive, we force people to either #ifdef by version, or to disable the warnings in all their builds and only be told later by an error (in which case we do not need the warnings). I think both alternatives have a good potential to frustrate users. If we are too soft, we have a maintainability problem, and this is not what I want. That's why I am in favor of an intermediate level for central functionality that has the potential to raise dozens of warnings (and make users disable the warning, rather than fix it). Of course, what is central is subjective and the decision puts a maintainability burden on us. I see me doing less work on doing this additional step in deal.II than in my projects using deal.II, but I realize that you @masterleinad have done much more cleaning work than me, so I admit that I'm not in a neutral position on this topic.

As it seems I am the only person arguing this way, I assume that it is my own frustration about work load and not a general problem, so I better keep silent on this now.

@kronbichler I see your point and agree with you concerns (I might have been too quick in deprecating since I got the impression that that is the normal: DoFHandler::initialize() , VectorizedArray::n_vector_entries). I think we were considering a version number of the DEAL_II_DEPRECATED macro. Maybe it is a good time to refresh the discussion there. I have a (maybe naive) implementation which might work (and could be configured at compile time via DEAL_II_DEPRECATED_VERBOSITY):

#define DEAL_II_DEPRECATED_VERBOSITY 0 #define DEAL_II_DEPRECATED "[[deprecated]]" #define CAT(a, ...) PRIMITIVE_CAT(a, __VA_ARGS__) #define PRIMITIVE_CAT(a, ...) a ## __VA_ARGS__ #define DEAL_II_DEPRECATED_VERSION(major,minor) PRIMITIVE_CAT(DEAL_II_DEPRECATED_VERSION_, major ## _ ## minor) #if DEAL_II_DEPRECATED_VERBOSE #define DEAL_II_DEPRECATED_VERSION_9_3 DEAL_II_DEPRECATED #else #define DEAL_II_DEPRECATED_VERSION_9_3 "" #endif #define DEAL_II_DEPRECATED_VERSION_9_2 DEAL_II_DEPRECATED #include <iostream> int main() { std::cout << DEAL_II_DEPRECATED_VERSION(9,2) << std::endl; // [[deprecated]] std::cout << DEAL_II_DEPRECATED_VERSION(9,3) << std::endl; // }

... this was the best I could come up with the help of https://github.com/pfultz2/Cloak/wiki/C-Preprocessor-tricks,-tips,-and-idioms. I would have loved if I didn't need to write out all version but could use the logic of:

# define DEAL_II_MPI_VERSION_GTE(major,minor) \ ((DEAL_II_MPI_VERSION_MAJOR * 100 + DEAL_II_MPI_VERSION_MINOR) >= (major)*100 + (minor))

We need to support projects where people install "deal.II master as of two weeks ago" and "deal.II master as of two months ago" without printing thousands of deprecation warnings so I agree with @kronbichler's general point (its why I closed #10335).

I support doing versioned deprecations. It wouldn't be that hard to have another level of deprecation that can be turned on or off that we promote to DEAL_II_DEPRECATED the moment we increment the version number.

Edit: a clarification: even having #ifdefs isn't enough to fix the problem where some things are deprecated on the current version of master and not deprecated on older versions so, even if users are willing to write dozens of them (which they shouldn't have to do), its not a fix.

I would be fine with defining a DEAL_II_DEPRECATED_EARLY macro that we promote to DEAL_II_DEPRECATED with the next release.

Maybe something like #11472?

bangerth · 2021-01-05T04:02:35Z

include/deal.II/lac/la_parallel_vector.h

@@ -605,10 +605,23 @@ namespace LinearAlgebra
       * After calling this method, read access to ghost elements of the
       * vector is forbidden and an exception is thrown. Only write access to
       * ghost elements is allowed in this state.
+       *
+       * @deprecated Use zero_out_ghost_values() instead.
       */
      void
      zero_out_ghosts() const;


bangerth · 2021-01-05T04:02:47Z

include/deal.II/lac/la_parallel_vector.h

       */
      void
      zero_out_ghosts() const;

+      /**
+       * This method zeros the entries on ghost dofs, but does not touch
+       * locally owned DoFs.


same comments about the documentation of this function

peterrum · 2021-01-16T21:05:25Z

I have updated this PR so that it used DEAL_II_DEPRECATED_EARLY. Thank you @masterleinad!

masterleinad · 2021-01-19T23:21:33Z

/jenkins/workspace/dealii_PR-11461/tests/simplex/compute_point_locations_01.cc: In function ‘void test_in_unit_cube(const std::vector<dealii::Point<dim> >&)’:
/jenkins/workspace/dealii_PR-11461/tests/simplex/compute_point_locations_01.cc:37:3: error: ‘GridGenerator’ has not been declared
   GridGenerator::subdivided_hyper_cube_with_simplices(tria, 1);
   ^

peterrum · 2021-01-20T08:34:32Z

@masterleinad That has been fixed on master a couple of days ago. ~~Should I rebase this PR nevertheless?~~

peterrum added Parallel distributed ready to test labels Jan 4, 2021

kronbichler reviewed Jan 4, 2021

View reviewed changes

peterrum force-pushed the zero_out_ghost_values branch from 15ddc29 to 79111f1 Compare January 4, 2021 12:15

bangerth reviewed Jan 5, 2021

View reviewed changes

masterleinad mentioned this pull request Jan 5, 2021

Introduce DEAL_II_DEPRECATED_EARLY #11472

Merged

peterrum force-pushed the zero_out_ghost_values branch from 79111f1 to a1ef57e Compare January 16, 2021 21:03

Rename zero_out_ghosts to zero_out_ghost_values

583015a

peterrum force-pushed the zero_out_ghost_values branch from a1ef57e to 583015a Compare January 20, 2021 10:14

kronbichler approved these changes Jan 20, 2021

View reviewed changes

masterleinad approved these changes Jan 20, 2021

View reviewed changes

kronbichler merged commit 50e0b19 into dealii:master Feb 9, 2021

masterleinad mentioned this pull request Jun 30, 2023

Remove deprecated Vector member functions #15534

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename zero_out_ghosts() to zero_out_ghost_values() #11461

Rename zero_out_ghosts() to zero_out_ghost_values() #11461

peterrum commented Jan 4, 2021

kronbichler Jan 4, 2021 •

edited

peterrum Jan 4, 2021 •

edited

bangerth Jan 5, 2021

bangerth Jan 5, 2021

bangerth Jan 5, 2021

bangerth Jan 5, 2021

masterleinad Jan 5, 2021

kronbichler Jan 5, 2021 •

edited

peterrum Jan 5, 2021

drwells Jan 5, 2021 •

edited

masterleinad Jan 5, 2021

masterleinad Jan 5, 2021

bangerth Jan 5, 2021

bangerth Jan 5, 2021

peterrum commented Jan 16, 2021

masterleinad commented Jan 19, 2021

peterrum commented Jan 20, 2021 •

edited

Rename zero_out_ghosts() to zero_out_ghost_values() #11461

Rename zero_out_ghosts() to zero_out_ghost_values() #11461

Conversation

peterrum commented Jan 4, 2021

kronbichler Jan 4, 2021 • edited

Choose a reason for hiding this comment

peterrum Jan 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kronbichler Jan 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwells Jan 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterrum commented Jan 16, 2021

masterleinad commented Jan 19, 2021

peterrum commented Jan 20, 2021 • edited

kronbichler Jan 4, 2021 •

edited

peterrum Jan 4, 2021 •

edited

kronbichler Jan 5, 2021 •

edited

drwells Jan 5, 2021 •

edited

peterrum commented Jan 20, 2021 •

edited