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

fix: performancewarning #376

Closed
wants to merge 4 commits into from
Closed

Conversation

huangziwei
Copy link
Contributor

fix #375. All tests passed.

It also seems just fixing the warning from base.py will also fix (or delay) the warnings in other places.

michaeldeistler and others added 2 commits July 1, 2024 16:58
* Allow continuing a simulation

* Feedback

* fixups
@huangziwei huangziwei changed the base branch from main to data_clamp July 4, 2024 08:40
Copy link
Contributor

@michaeldeistler michaeldeistler 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 not merging into main, right?

@huangziwei
Copy link
Contributor Author

I think it's better merged to data_clamp for now.

@kyralianaka
Copy link
Contributor

kyralianaka commented Jul 5, 2024

Thanks @huangziwei! Unfortunately the latter 2 bullet points in #375 are still triggering the performance warning for me though
Update: just kidding, performance warning gone :)

@kyralianaka
Copy link
Contributor

The performance warning for adding channels is fixed though!

@huangziwei
Copy link
Contributor Author

I guess it's device dependent (My laptop has 16Gb RAM), and there's no warning when I ran your retina notebook. It's also why I said the fix likely just delays the warning...

can you try replacing

            view["global_comp_index"] = view["comp_index"]
            view["global_branch_index"] = view["branch_index"]
            view["global_cell_index"] = view["cell_index"]

in network.py with

            global_indices = pd.DataFrame(
                {
                    "global_comp_index": view["comp_index"],
                    "global_branch_index": view["branch_index"],
                    "global_cell_index": view["cell_index"],
                }
            )
            view = pd.concat([view, global_indices], axis=1)

?

@michaeldeistler
Copy link
Contributor

Just do this at the beginning of your script to suppress the warning:

import pandas as pd
from warnings import simplefilter
simplefilter(action="ignore", category=pd.errors.PerformanceWarning)

@huangziwei
Copy link
Contributor Author

I think the concern is some pandas operations used up too much RAM, so we should handle them in a more efficient way.

@michaeldeistler
Copy link
Contributor

are we really having problems with this or is it just the warning? I don't think that pandas is a limiting factor to runtime performance of Jaxley.

@kyralianaka
Copy link
Contributor

It is not a problem, just a low-priority thing to improve. Anyone who uses groups in Jaxley would probably get severely annoyed by the warning and have to silence it though. The warning is actually gone now for me though thanks to the changes!

@michaeldeistler
Copy link
Contributor

I am not sure it has to do with groups, just with lots of mechanisms

@kyralianaka
Copy link
Contributor

Yes I think you're right

@huangziwei
Copy link
Contributor Author

I am closing this as the major patches has been merged into the data_clamp branch and the latest commit has been addressed by another PR #384

@huangziwei huangziwei closed this Jul 18, 2024
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.

Pandas performance warning
3 participants