MxUI / MUI

Multiscale Universal Interface: A Concurrent Framework for Coupling Heterogeneous Solvers
http://mxui.github.io/
Apache License 2.0
54 stars 40 forks source link

Add sub-iteration capability #65

Closed SLongshaw closed 4 years ago

SLongshaw commented 4 years ago

Adding ability to define time in terms of main and sub-iteration (while retaining capability to use a single value for time)

yhtang commented 4 years ago

Hi Stephen,

I thought, perhaps, that the sub-iteration capability could be implemented with a bit less effort. Since the filter method of chrono-samplers are templaterized on the data type of the time stamp, could we just define a new time_type, instead of overloading the filter methods? We could then overload partial order comparator operator < and so on to implement proper interpolation behaviors. Hopefully, this could avoid the existence of methods such as filter and filter_si which feels a bit bloaty.

These are just my preliminary thoughts. It's totally possible that there are design factors that I didn't quite get at first. Please just let me know what you think. Thanks!

Best, Yu-Hang

SLongshaw commented 4 years ago

Hi Yu-Hang,

I fully agree, my implementation is very much a "quick and dirty" effort to add the capability for a project I am currently using MUI in. For the moment I just needed the ability to include sub iteration in the time stamping (I haven't included it in the smart send stuff you'll note, that just uses the main time value still).

More than happy for what I have done to be implemented in a neater way for sure!

My priority at the moment is the project I am working on but if I get some time to look at this again I will have a go at what you suggest. I've left it in the dev branch for now and won't migrate it through to master until it's neater. Of course more than happy for somebody else to have a go at re-implementing also!

I think one issue I was struggling with is if a compound data type is defined for time_type (i.e. one with two time values contained within) then what does that do to all of the methods in uniface.h that directly use these values such as fetch, commit, barrier etc. Some sort of logic based around the time_type could be defined to decide whether there is 1 or 2 values to be used but the way I have implemented it was just the quick method around this problem and even then this would add extra logic to the existing methods.

Of course I might be missing a really obvious solution to this!

Kind regards,

Stephen

yhtang commented 4 years ago

Hi Stephen,

Thanks for the detailed explanations. I am thinking about whether we can just define something like

template<class T_MAJOR, class T_MINOR>
struct time_subiterations {
    T_MAJOR major;
    T_MINOR minor;

    // define partial order
    bool operator <;

    // define arithmetics
    bool operator +;
};

Of course, there might a catch here: within a step, a newer subiteration might need to override all previous ones, which only represent intermediate results. Is this something that you do encounter in your specific application?

SLongshaw commented 4 years ago

Hi Yu-Hang,

Sorry for the delay in replying.

I don't think the need to override is something to worry about, certainly in my current application a time stamp represented by a major and minor value for time is instantaneous, so can be considered unique compared to others.

I'm probably missing what you mean exactly but I'm still a little unsure how this implementation would work without adding extra logic/overloaded functions as time_type is considered to be a singleton in many areas of the original code , if it's a compound type as per your time_subiterations struct then how would this work?

If you do have the time I would really be interested to see your proposed implementation, even if only partial, perhaps you could do it within a fork of the current master branch?

Thanks for your interest, adding this capability is actually fairly important and something a number have asked me about over the past few years and we have just skirted around it rather than solving it...

SLongshaw commented 4 years ago

Just to make a quick new comment on this.

I have spent some time looking into this and have come to the conclusion that it is genuinely quite difficult to introduce this functionality in a much neater way than I already have without breaking backwards compatibility (i.e. changing the parameter ordering of important functions like fetch()).

I have logically tried introducing a new compound time data type as you suggested in a few ways, either using a std::pair or std::tuple and also by creating a new data type much like the point class (effectively what you suggested earlier in this thread). The problem in every case is that, at some point in the various functions that rely on time_data (these are both the core functions and data structures in uniface.h as well as some smaller functions in util.h), you run into an issue where the logic of having either one or two values just makes things complicated.

As a (hopefully acceptable) middle ground I have re-implemented my original attempt in a somewhat neater fashion so there is at least only 1 "log" structure. The downside is that this means that even in cases where people are only using a singleton for the time stamp, a second value is still sent, set logically to ensure the map searches work as expected, this increases network traffic a little. The only way to avoid this is to go back to a full logical split in the data structures and receiving functions, so I think the trade-off is acceptable.

Some function overloading is unavoidable as long as I don't want to break backwards compatibility, so there are still multiple versions of fetch and there are now two filter functions in the temporal samplers (this is to avoid adding a branching statement into a function where performance is important, the alternative would be to template the function but I think this is more messy than just using function overloading). This could be avoided if I could re-order parameters so I could define a default value for the second "time" parameter but I would rather just use overloading than break any code already built on top of MUI.

Let me know what you think, if you can think of an easy and neat way of introducing this functionality using a sensible structure that means we don't introduce the unnecessary second value in cases of using a single time value rather than a pair then I am more than open to re-working this. Perhaps I'm looking at this all wrong