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

Add Raith MBMS Paths #255

Closed
wants to merge 12 commits into from
Closed

Conversation

MatthewMckee4
Copy link

from #252. I have implemented mbms path functionality into the FlexPath Object (raith_data and base_cell_name), and added support for reading and writing to and from gds. I have also added more appropriate type hints to the gdstk.pyi file.
These changes should not affect any pre-existing functionality.

@heitzmann
Copy link
Owner

Thanks @MatthewMckee4 ! That's quite the effort. I'll try to go over your changes this weekend!

Copy link
Owner

@heitzmann heitzmann left a comment

Choose a reason for hiding this comment

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

This is a great implementation, thanks @MatthewMckee4 for sticking to the source standards! I've added a few comments throughout the PR, if you could take a look.

Also, I'm assuming you've tested this implementation with the Raith software, right? I have no way of testing this myself except for bare minimum correctness.

include/gdstk/flexpath.hpp Outdated Show resolved Hide resolved
python/raithdata_object.cpp Show resolved Hide resolved
python/raithdata_object.cpp Outdated Show resolved Hide resolved
src/flexpath.cpp Outdated Show resolved Hide resolved
src/flexpath.cpp Outdated Show resolved Hide resolved
include/gdstk/utils.hpp Show resolved Hide resolved
src/flexpath.cpp Outdated Show resolved Hide resolved
src/flexpath.cpp Outdated Show resolved Hide resolved
include/gdstk/utils.hpp Outdated Show resolved Hide resolved
src/raithdata.cpp Outdated Show resolved Hide resolved
@MatthewMckee4
Copy link
Author

Thank you, this is my first time working with c++ tbh so I'm glad to hear it's not horrible. I will get these changes updated asap.

@MatthewMckee4
Copy link
Author

@heitzmann is there any chance of this getting merged soon? I'm currently under some pressure at work to get this done so if you have a timeline that would be greatly appreciated.

@heitzmann
Copy link
Owner

I saw a few pushes so I was not sure you were finished with the changes. If you are, we can merge it already!

@MatthewMckee4
Copy link
Author

Yeah, I'm all done with my changes now. Thanks.

@heitzmann
Copy link
Owner

Tests are failing with a segfault. Have you tested locally, @MatthewMckee4 ?

@MatthewMckee4
Copy link
Author

I was having some issues so I didn't manage to test much, my apologies. I will get these issues fixed asap

@MatthewMckee4
Copy link
Author

MatthewMckee4 commented Jul 2, 2024

@heitzmann I am unable to reproduce the issues on my device but from the logs, I can see some issues:
image
I have fixed the issue on line 160, this was simply me writing the wrong method. The other issue though comes from:
image
where dwelltime_selection is:
image
would I be right in changing this to:
image
or
image
If not whats the correct casting function?

@heitzmann heitzmann closed this in 2c963ba Jul 4, 2024
@heitzmann
Copy link
Owner

@MatthewMckee4 I went over all the changes and made a few modifications I thought necessary. Could you please test commit 2c963ba to make sure it works? I don't have a way to test the Raith data locally. If all's working, we can create a new release with the new feature. And thanks again for the push!

@MatthewMckee4
Copy link
Author

@heitzmann this passes all of our tests. Should be good to merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants