Remove new forest from adapt callback arguments

[lukasdreyer]

Feature request

Remove non committed new forest from adapt callback, see: https://github.com/DLR-AMR/t8code/blob/facc24c067dfa0b4299af4919a064e3a9770c2e0/src/t8_forest/t8_forest_general.h#L112-L113

Estimated priority Which of these is most applicable (remove the others):

"Priority: medium" Should be solved within half a year

Additional context Add any other context or screenshots about the feature request here.

[dutkalex]

Hi! This is indeed a good idea IMO, as the current API is really confusing and I really can't think of a valid usecase for querying the not-yet-commited forest. I would just like to advocate for this breaking change to be handled through an explicit deprecation process to give users a little bit of time and flexibility. I don't know your policy regarding C++ standards, but if you could ship with both the new API (without the not-yet-commited forest) and the old one, with a [[ deprecated ( "please migrate to the new API" ) ]] attribute (which requires C++14 IIRC), that would be great :)

[spenke91]

I created a draft PR #1732 removing the forest argument from the adapt callback (but not yet the deprecated keyword). As you can see in the changed files there, in a lot of the cases in benchmarks, example, test, and tutorials, the adaptation callback actually do use the forest argument. Typically, to call t8_forest_get_user_function or to access forest->t8code_data, as in

or

Since I am still rather unexperienced with this, my question is: Would you (e.g., @holke, @lukasdreyer, @sandro-elsweijer, @dutkalex ) consider this a usecase for which we should keep the forest argument, or should I adjust the code such that the user data is passed via the forest_from argument? 🤔 The latter option is possible, I tested it locally, but it does not really seem clean because it means modifying forest_from or forest->set_from e.g., in t8_forest_new_adapt or t8_forest_adapt...

[holke]

thanks for looking into this. Imho in these use cases it should be fine switching to putting the user data on to forest_from. The purpose of the user data functions is to temporarily add some data to a forest that is needed in callback functions or elsewhere. So this would be a perfect use case also for forest_from.

I tested it locally, but it does not really seem clean because it means modifying forest_from or forest->set_from e.g., in t8_forest_new_adapt or t8_forest_adapt...

i do not really understand what you are saying here and what the issue is. Why would we need to modify forest_from?

[dutkalex]

Would you (e.g., @holke, @lukasdreyer, @sandro-elsweijer, @dutkalex ) consider this a usecase for which we should keep the forest argument, or should I adjust the code such that the user data is passed via the forest_from argument? 🤔

Personally, I would not label that as a "usecase" in the sense that it does not serve a user need. I would rather categorize that as an implementation need because it is used by internal APIs only. I am fairly convinced that there is no situation in which client code can leverage this exposed forest since it in the process of being constructed when the callback is invoked. Its state can therefore not be reasoned about without additional knowledge about t8code's inner workings, which are definitely not supposed to be relied upon by client code.

The latter option is possible, I tested it locally, but it does not really seem clean because it means modifying forest_from or forest->set_from e.g., in t8_forest_new_adapt or t8_forest_adapt...

If I may voice my outsider opinion on the matter, attaching the callback to a particular forest (whether the old or the new one) feels quite unnatural because this is fundamentally not a property of the forest. If you choose to attach it to the original forest, then the scenario of successively adapting the same forest in two different ways (attaching the original forest with the first callback to launch a first adaptation, and then replacing the callback to perform another adaptation) feels wrong. It you choose to attach it to the new forest, there is fewer opportunities for weird code since the forest is conceptually immutable once commited, but keeping around the callback that once served to build it still feels strange too IMO. The adaptation callback is its own thing, that belongs neither with the original forest nor with the new one, because it is the piece of logic that transforms one into the other. In other words, it is effectively meaningless outside of the context of the adaptation call: any use outside of this context is surely a bug and it should not be attached to any of the forest because they outlive the adaptation call context. I would argue that the std::transform algorithm is a good example of how to deal with such algorithm customization points: the callable parameterizing the behavior of the algorithm is not bound to the input not the output but passed as a separate argument, and the API is crafted to encourage the use of rvalues (often lambdas) so that the customization point's lifetime is bound to the algorithm call context. Here, the reasoning extends beyond just the callback function: the flags telling whether the adaptation should balance, partition and compute the ghost elements are other examples of the same thing. I would therefore argue that there is a latent abstraction here waiting to be unearthed (something like an "adaptation strategy") that would encapsulate the callback and the different flags. This discussion goes obviously beyond the scope of this particular issue, but I think it is helpful to think of this implementation difficulty not as an isolated issue but rather as a symptom of something larger.

[holke]

Thank you for sharing this opinion @dutkalex :)

[spenke91]

thanks for looking into this. Imho in these use cases it should be fine switching to putting the user data on to forest_from. The purpose of the user data functions is to temporarily add some data to a forest that is needed in callback functions or elsewhere. So this would be a perfect use case also for forest_from.

I tested it locally, but it does not really seem clean because it means modifying forest_from or forest->set_from e.g., in t8_forest_new_adapt or t8_forest_adapt...

i do not really understand what you are saying here and what the issue is. Why would we need to modify forest_from?

Sorry if I wasn't clear. I meant that for example in t8_forest_new_adapt, the passed user data is written into forest, i.e.,

If we'd just add it to forest_from, the function would modify forest_from, which may seem counter-intuitive.

There may be a more elegant solution though. Honestly, I just wanted to make sure this does not render the whole point of this issue obsolete before putting more thoughts into it. So thank you both for your answers! :-)

[spenke91]

I looked a little bit into how to treat forest->user_data and forest->t8code_data once we remove the forest argument from t8_forest_adapt_t. In principle, all test cases are running fine if I make sure to pass on the pointers from forest to forest_from before calling forest->set_adapt_fn, like this:

However, to me this does not feel like a clean solution, because it propagates information from forest to forest->set_from, which seems like the opposite direction of what's intended.

So far, I considered two main alternatives: (A) Since we modify the interface of the adaption callback anyway, me could add an argument user_data here, i.e., When calling the adaptation, we would then pass the forest's user_data, i.e., This would seem cleaner to me, because it would still use the user data of the curent forest without exposing the forest to the adaptation callback. However, this solution only works for forest->user_data, but there is also one adaptation callback (t8_forest_balance_adapt) using forest->t8code_data.

(B) We could also ensure that all functions creating a new forest based on an existing one (or maybe t8_forest_commit would suffice) also propagate the user_data pointer from the existing to the new forest, i.e., in the correct direction. However, (a) we might accidentally miss a function here and (b) it again would only work for the user_data where we can ensure / demand that it is set for an existing forest before creating new ones based on that.

In both cases, I willtry to come up with a clean solution for forest->t8code_data

-> What do you think? Of course, I might be getting something wrong or missing something, but I still wanted to share my unfinished thoughts here before the parental leave. Maybe it is also a good topic to discuss in a developer meeting when I am back...

[holke]

Edit: Just realized that this is basically what you wrote before.

following our discussion today, @spenke91 these were my thoughts:

• remove the new forest from the parameter list of the callback.
• add void *user_data to the parameter list of the callback.
• add void *user_data to the parameter list of t8_forest_set_adapt
• internally buffer the user_data pointer somewhere (for example a new adapt_data pointer in the (old) forest) and pass it onto the adapt callback:

MWE with one double as user data:

int user_adapt_callback (t8_forest_t forest_from, t8_locidx_t which_tree,
                                  const t8_eclass_t tree_class, t8_locidx_t lelement_id, const t8_scheme_c *scheme,
                                  const int is_family, const int num_elements, t8_element_t *elements[], void *user_data)
{
  const double my_user_data = *(double*)user_data;
  // do something
}

// more code here
doubt my_user_data = 42;
t8_forest_set_adapt (forest_new, forest_from, adapt_callback, recursive, &my_user_data); 
t8_forest_commit (forest_new);

Since it is our mission to get rid of void* (at least in the C++ code base), maybe we can do better if we could make it typesafe by using templates in some way... Maybe a new adapt class that can handle the adaptation...?

template <typename T>
class adapt_handler {
  
     // constructor getting old forest, new forest, adapt_callback and user data
     adapt_handler  (t8_forest_t, t8_forest_t, ADAPT_CALLBACK_POINTER,T&);
     // carry out the adaptation algorithm   
     int do_adapt();


     // typesafe callback function with T as user data type
     int (*user_adapt_callback) (t8_forest_t forest_from, t8_locidx_t which_tree,
                                          const t8_eclass_t tree_class, t8_locidx_t lelement_id, const t8_scheme_c *scheme,
                                          const int is_family, const int num_elements, t8_element_t *elements[], const T &user_data);
   // Could this even be a virtual function and user implement a class thats inherit? May be better, but usually virtual functions and templates dont mix... so might need to stick with the function pointer
   
     t8_forest_t forest_from; // forest to adapt
     t8_forest_t forest_new; // forest to be constructed via adapt
     
     // user data
     T &user_data;
         
};

This will require some thought though how to put it into the "set" and "commit" logic...since the templated class could not be stored in the forest itself, otherwise we would have to templetize the forest...