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

Tvb cosim #99

Closed
wants to merge 473 commits into from
Closed

Tvb cosim #99

wants to merge 473 commits into from

Conversation

lionelkusch
Copy link
Collaborator

Add two monitors for interface with TVB :

The simplest is Interface_co_simulation_parallel.py. The input is the state variable and the output is the state of other node and the input of proxy node.

The second monitor is Interface_co_simulation.py. The input is intermediate result. This monitor is need some modification of the model in order to include this intermediate result.

@maedoc
Copy link
Member

maedoc commented Apr 14, 2020

should I be able to test this with the current version of NEST or I need the latest/master?

@lionelkusch
Copy link
Collaborator Author

This version is independent of Nest or other simulator.
The tests are based on the TVB coupled with TVB or some specific function.

There are a proof of concept based on this interface and Nest. The proof of concept use MPI for the communication. In consequence, you need a wrapper over this interface for the MPI communication.
This implementation is base on the repertory : https://gitlab.version.fz-juelich.de/diaz1/co-simulation-tvb-nest.git
The version of Nest is the following version : nest/nest-simulator#1456
This pull request is the creation of Input and Output for Nest with MPI.
If you need an access to the proof of concept, you should send me an email ( lionel.kusch@univ-amu.fr)

liadomide and others added 27 commits May 4, 2020 01:48
TVB-2697: Fixed displaying PSE count message for guid ranges
TVB-2682: Set a specific default conductionSpeed in order for the spa…
TVB-2676: Solved problem with display_name
…iling because he import of operations still relies on operation.xml and does not correctly handle ViewModel...h5 (it is treated as DT).
TVB-2684 Remove FlowService.get_available_datatypes method, after review of unit-tests where it was used
…n using .column_attrs.keys() and remove 'type'
…ivity Visualizer problem for simulations with surface.
TVB-2684 TVB-2679 Review FlowService, add ValueWrapper, unit-tests fixes
TVB-2689: add summary_info to DataType indexes
…werForm.get_filters

TVB-2696 Make sure errors in filters are actually caught individually and the overlay appears
Test need to be improve with the system of test.
The buffer has some problems.
I add a test for checking again this problems.
@lionelkusch
Copy link
Collaborator Author

I update master and fix a bug in one of interface

@lionelkusch
Copy link
Collaborator Author

The public project of the co-simulation between Nest and TVB is as the following link :
https://github.com/multiscale-cosim/TVB-NEST

@liadomide
Copy link
Member

This PR is not easy to inspect (too many files appear changed). It would have been better to rebased towards master.
Could you cherry pick your commits and put them on top of the current master ?

NEED to add test for testing the model with more state variable and the
model with varaible not part of coupling variable
Copy link
Member

@liadomide liadomide left a comment

Choose a reason for hiding this comment

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

I have some cosmetic changes mentioned bellow (you should choose what to implement or not), but my main concern is that the current diff is impossible to assess, due to many unrelated changes highlighted.

@@ -0,0 +1,83 @@
# Copyright 2020 Forschungszentrum Jülich GmbH and Aix-Marseille Université
# "Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements; and to You under the Apache License, Version 2.0. "
Copy link
Member

Choose a reason for hiding this comment

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

Some files recently introduced differ in license from tvb-library GPL license.
While we have no problem with Apache license, and its compatibility, I am wondering if we could not keep consistency inside tvb-library and adhere to the same license, to be uniform (and from my perspective easier to present/package/distribute).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I forget to change the licence.


print("time_synchronize : %r s"%(time_synchronize*10**-3))
print("max error S rate for 1 : %r" % max_error_s_1)
print("max error S rate for 2: %r" % max_error_s_2)
Copy link
Member

Choose a reason for hiding this comment

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

This is not really a unit-test, right?
Could you make it pytest conform ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it was some print for helping me to debug the code. I am modify all the test for be in the right format


print("time_synchronize : %r s"%(time_synchronize*10**-3))
print("max error S rate for 1 : %r" % max_error_s_1)
print("max error S rate for 2: %r" % max_error_s_2)
Copy link
Member

Choose a reason for hiding this comment

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

Also not a proper pyunitest.
print calls should not happen


print("time_synchronize : %r s"%(time_synchronize*10**-3))
print("max error S rate for 1 : %r" % max_error_s_1)
print("max error S rate for 2: %r" % max_error_s_2)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe there are not really meant as unit-tests and should rather be examples, e.g. under tvb_documentation/demos ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Most of these test can be also use for demonstration.
We need to decide what is the most appropriate for the demonstration.

Copy link
Member

Choose a reason for hiding this comment

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

I think both aspects should be covered.
under tests we should have automated checks, to be run with every change of the code, and ensure regressions don't happen (too often), while demo notebooks would also be great to have (under tvb_documentation would be my recommendation).

s = result[0][1][:,0]
return time,s

class tvb_sim:
Copy link
Member

Choose a reason for hiding this comment

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

Could you pls adhere to the conventions: module (file names) should start with lowercase and class names be camel case ?
from tvb.simulator.interface_co_simulation_parallel import InterfaceCoSimulation
class TVBSim:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I will refactore the code for the respect this convention

@lionelkusch lionelkusch mentioned this pull request Jul 9, 2020
@lionelkusch
Copy link
Collaborator Author

I close the Pull Request because I don't know how to fix my error due to the update of master.
I create a new one (#190) with all the comments already include for continuing.
Sorry for the inconvenient.

@lionelkusch lionelkusch closed this Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants