Reimplement AnalysisResultTable #1252
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… columns are managed by a mix-in. Remove baseclass ThreadSafeDataFrame having both responsibility since this is only used for analysis results.
|
This must be merged before 0.6 otherwise we will introduce breaking API change. |
itoko
reviewed
Aug 24, 2023
| import pandas as pd | ||
|
|
||
|
|
||
| class DefaultColumnsMixIn: |
There was a problem hiding this comment.
This class seems not designed to be used in different places (the interface heavily depends on Pandas Dataframe and if we change the implementation of container, this class will be changed accordingly). I think it would be good to notify users not to use (subclass) this class in other places.
There was a problem hiding this comment.
Thanks. This is good suggestion. Done in 9b3bff9
| **kwargs, | ||
| ) | ||
|
|
||
| def drop( |
There was a problem hiding this comment.
I'd prefer drop_entry (To me, drop feels it implements all features supported in pd.Datafram.drop)
There was a problem hiding this comment.
Makes sense. Done in 6925655
nkanazawa1989
added a commit
to nkanazawa1989/qiskit-experiments
that referenced
this pull request
Jan 10, 2024
### Summary Follow-up of qiskit-community#1133 for data frame. ### Details and comments I realized I had some mistake when I wrote a base class of `ThreadSafeDataFrame`. We plan to move the curve data (XY data) to the artifact for better reusability of the data points. In addition, [CurveData](https://github.com/Qiskit-Extensions/qiskit-experiments/blob/c66034c90dad73d705af25be7e9ed9617e7eb2ef/qiskit_experiments/curve_analysis/curve_data.py#L88-L113) object which is a container of the XY data is also replaced with the data frame in a follow up PR. Since this table should contain predefined columns such as `xval`, `yval`, `yerr`, I was assuming this container could be a subclass of `ThreadSafeDataFrame` -- but this was not a right assumption. Since the curve analysis always runs on a single thread (analysis callbacks might be run on multiple threads and thus `AnalysisResultTable` must be thread-safe), this curve data frame doesn't need to be thread-safe object. In this PR, a functionality to define default columns of the table is reimplemented as a Python mixin class so that the function to add default columns will be used also for curve data frame without introducing the unnecessary thread safe mechanisms. `AnalysisResultTable` is a thread safe container but the functionality to manage default columns is delegated to the mixin.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up of #1133 for data frame.
Details and comments
I realized I had some mistake when I wrote a base class of
ThreadSafeDataFrame. We plan to move the curve data (XY data) to the artifact for better reusability of the data points. In addition, CurveData object which is a container of the XY data is also replaced with the data frame in a follow up PR. Since this table should contain predefined columns such asxval,yval,yerr, I was assuming this container could be a subclass ofThreadSafeDataFrame-- but this was not a right assumption.Since the curve analysis always runs on a single thread (analysis callbacks might be run on multiple threads and thus
AnalysisResultTablemust be thread-safe), this curve data frame doesn't need to be thread-safe object. In this PR, a functionality to define default columns of the table is reimplemented as a Python mixin class so that the function to add default columns will be used also for curve data frame without introducing the unnecessary thread safe mechanisms.AnalysisResultTableis a thread safe container but the functionality to manage default columns is delegated to the mixin.