-
Notifications
You must be signed in to change notification settings - Fork 5
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
ROEChargeInjection does not work via wrapper #19
Comments
I have got it working via the following hacks, which are on the branch
Is now:
Anddddddddddddddddddd
Is now:
|
Great, thanks. If this works, please merge it. |
The inheritance structure of the original code was broken, my solution involved a lot of copy and pasting doing bad thing... Happy to merge it but its not good code and I would assume other parts of the code might be broken? |
Oh, OK I see - sorry. This might be one to ask Jacob about. Could be really easy for him to fix. |
I should be able to look at this during next week if that's okay. |
I don't think the problem is happening for me? Sorry it took me a while to check it out. And perhaps I'm just missing something. But I'm on the latest So can you check that on yours, making sure you're not on an older branch, and if it is broken there or breaks with a different test case then send me a MWE and I'll take another look :) |
Definitely not fixed on the I am running this script, and have checked via my hack that the output likelihood should give The methods of |
I'm confused, why are you using master, the latest version which is where I'm saying it seems to work is |
master and dev look in sync (I think Richard maybe did a merge?) to me, but I will try dev in a bit. |
Oh okay, perhaps he did. Could you try doing something more explicit like my check above? I don't know what those numbers you printed mean. And if there is still a problem, I have a conference this coming week but if you could make a more minimal example then I can try to have a look. |
I merged them today, because everything else was working.
Having said that, I also just found a bug, in that charge is not conserved in an image - either the FPR is too small or the EPER too big (which might explain James’s amazing recent results). I’m investigating that separately/at the moment.
On 2 Mar 2022, at 15:30, Jacob Kegerreis ***@***.******@***.***>> wrote:
[EXTERNAL EMAIL]
Oh okay, perhaps he did. Could you try doing something more explicit like my check above? I don't know what those numbers you printed mean. And if there is still a problem, I have a conference this coming week but if you could make a more minimal example then I can try to have a look.
—
Reply to this email directly, view it on GitHub<#19 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AE5BZNS5PRMQBQSFXQGPC4TU56CRRANCNFSM5K4AF7XQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you commented.Message ID: ***@***.***>
|
For me, the following script gives 0.0 at the bottom, meaning that the addition of CTI is identical for the two ROE's:
Do you get the same behaviour? |
Okay thanks that looks clear, I'll have a look when I can, hopefully before the conference starts, but let me know if it's urgent. |
Got my hacked branch so not urgent :) |
This currently works perfectly on dev branch. But only because no classes are inherited from anywhere else (I think). For example, ROEChargeInjection() is its own class, and has nothing to do with ROE(). roe.cpp certainly looks very different from the snippet pasted above. Jacob, did you fix that? It works, but is it ideal? I can try to be more clever if that would be worthwhile. |
check https://en.cppreference.com/w/cpp/language/virtual |
If I pass ROEChargeInjection to the wrapper, it does not calls it
set_express_matrix_from_rows_and_express
andset_store_trap_states_matrix
methods, instead reverting to the methods of the defaultROE
class.I have put a
print(roe->type) before the following line in
add_cti.cpp`:roe->set_express_matrix_from_rows_and_express(n_rows, express, offset);
And the type is indeed type 1, indicating that the Python wrapper appears to be passing the correct type.
My best guess is that the inheritance structure is failing to overwrite the methods correctly?
The text was updated successfully, but these errors were encountered: