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

added first pass at python features #43

Closed
wants to merge 1 commit into from

Conversation

mgeplf
Copy link
Contributor

@mgeplf mgeplf commented Feb 15, 2016

* as desired in:
    BlueBrain#27
* currently only for Python
* Documentation will come as we settle on an API
def test_registerFeature(self):
import efel.cppcore

def getTwiceAP_Amp(output):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how to use it

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that ideally the feature should use setFeature to store it's own values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's mainly used as an internal cache, correct?
I could keep that local to cppcore.cpp, if that makes sense. What is the invalidation policy for the cache at the moment (so I can mirror it)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not 100% sure, but I think that cppcore.Initialize resets the cache ?

@codecov-io
Copy link

Current coverage is 100.00%

Merging #43 into master will not affect coverage as of 8584cc5

@@            master     #43   diff @@
======================================
  Files            5       4     -1
  Stmts          151     118    -33
  Branches         0       0       
  Methods          0       0       
======================================
- Hit            151     118    -33
  Partial          0       0       
  Missed           0       0       

Review entire Coverage Diff as of 8584cc5

Powered by Codecov. Updated on successful CI builds.

return len(feature_values)

self.setup_data()
efel.cppcore.registerFeature('getTwiceAP_Amp', getTwiceAP_Amp)
Copy link
Contributor

Choose a reason for hiding this comment

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

We would also need some mechanism to let different versions of the same feature live next to each other. Maybe we can add a version_string argument ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about just using the name of the feature do it: 'AP_amplitude_v1', 'AP_amplitude_v2', etc. That way, the calling convention doesn't have to change. Otherwise, getFeature has to take a version as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm, that won't be general enough. The idea is that the user should be able to replace e.g. AP_amplitude with it's own version, 'and' that then every feature that depends on AP_amplitude will pick up the new implementation automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok.

Then, the idea would be that features can still depend on the old version if that's what their dependency says? That means we have to version all features, including the ones that already exist, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, but that's why we have the dependency file and the liibv* versions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, AP_amplitude wouldn't be overwritten for those, just for future ones?

@wvangeit
Copy link
Contributor

wvangeit commented Jan 25, 2018

I will closing this until we have time to look at this again (have put an 'archived' label)

@wvangeit wvangeit closed this Jan 25, 2018
ilkilic pushed a commit that referenced this pull request Dec 8, 2023
Adjust amplitude use in_target for HyperDepol
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.

3 participants