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

Remove get_type func from RecordingDevice #2770

Closed
wants to merge 2 commits into from

Conversation

med-ayssar
Copy link
Contributor

@med-ayssar med-ayssar commented May 5, 2023

Remove the get_type function from the RecordingDevice class. The function is only required when using a RecordingBackEndMPI with a device different from a spike_recoder.

The function is also being used in RecordingBackEndSionLib to store information about the device, but the information itself is useless to the user reading data from the binary file created by RecordingBackEndSionLib.

@JanVogelsang
Copy link
Contributor

Could we think about/discuss a way to solve this at compile time? A function with the only function to solve type compatibility sounds a lot like traits. I have never used traits in practice though, only read about them, so I can't immediately make some working code suggestion. But I believe that the enroll function should be different depending on the traits of the RecordingDevice.

@med-ayssar
Copy link
Contributor Author

Any traits Magic must know concrete the type of the device, but we want to make the recordingbackend unaware of such concrete types.

@JanVogelsang
Copy link
Contributor

JanVogelsang commented May 9, 2023

Well, I disagree. Both RecordingDevices and RecordingBackends are known at compile time, so we should resolve their compatibility at compile time as well. That requires making the Backends aware of the concrete Devices they should support. Or not necessarily just support, but provide different implementations for different devices.

void
nest::RecordingBackendMPI::enroll( const RecordingDevice& device, const DictionaryDatum& )
{
  if ( device.is_recording_spikes() )
  ...
}
template< typename RecordingDeviceT >
struct is_recording_spikes< RecordingDeviceT > : std::false_type {};

template<>
struct is_recording_spikes< spike_recorder > : std::true_type {};

template< typename RecordingDeviceT >
void
nest::RecordingBackendMPI::enroll( const RecordingDeviceT& device, const DictionaryDatum& )
{
  if constexpr ( is_recording_spikes< RecordingDeviceT >::value )
  ...
}

The if constexpr() construct is a C++17 feature, so we would have to upgrade to C++17 before, but we decided to do that anyway already.

@med-ayssar
Copy link
Contributor Author

The problem that the enroll function is virtual function and virtual functions do not like template functions

@JanVogelsang
Copy link
Contributor

There are several ways around that. The easiest would be to just have a private enroll_ function which is not virtual, templated and called inside the enroll function. But there must be better ways. We have this problem at several points in our code, so it would make sense to think about this in more depth and come up with a good solution which we can then employ everywhere in the code. @jougs Any ideas or suggestions how we should proceed to find such a solution?

@terhorstd terhorstd added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know labels Jun 27, 2023
@med-ayssar
Copy link
Contributor Author

It is now irrelevant as we are planning to refactor the RecoridingDevice class.

@med-ayssar med-ayssar closed this Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants