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

Refactor names/folders/objects for better verbosity #5

Merged
merged 8 commits into from
Jun 6, 2024
Merged

Refactor names/folders/objects for better verbosity #5

merged 8 commits into from
Jun 6, 2024

Conversation

GalyaZalesskaya
Copy link
Contributor

@GalyaZalesskaya GalyaZalesskaya commented Jun 3, 2024

Methods/Objects:

  • task_type=TaskType - > task=Task
  • explain_method_type=XAIMethodType.RECIPROCAM -> explain_method=Method.RECIPROCAM
  • ExplanationResult -> Explanation
  • post_processing_parameters=PostProcessParameters -> visualization_parameters=VisualizationParameters
  • PostProcessor -> Visualizer
  • explanation.sal_map_shape -> explanation.shape
  • openvino_xai.explanation.explanation_parameters.py: SaliencyMapLayout -> openvino_xai.explainer.explanation.py: Layout
  • Added possibility of openvino_xai.Method and openvino_xai.Task
  • import openvino_xai as xai ... xai.Task, xai.Method for external usage
  • import openvino_xai as xai ... xai.Explainer for external usage

Files/Folders:

  • openvino.xai

    • explain -> explainer
      • explanation_parameters -> parameters
      • visualize -> visualizer
    • algorithms -> methods
    • insertion -> inserter
      • insertion_parameters -> parameters
  • Added 2024 year in LICENSE

@GalyaZalesskaya
Copy link
Contributor Author

To discuss @negvet @goodsong81

openvino_xai.insertion -> openvino_xai.xai_branch_inserter

Rename to be more concise? xai_branch_inserter | xai_inserter | inserter | xai_ir_inserter

@GalyaZalesskaya
Copy link
Contributor Author

We need to have consistency between 3 options of calling Method, Task:

import openvino_xai as ovxai

ovxai.Task, ovxai.Method
from openvino_xai import Task, Method

Task, Method
from openvino_xai.common.parameters import Task, Method

Task, Method

I personally like both 2 and 3 option equally.

@GalyaZalesskaya GalyaZalesskaya marked this pull request as ready for review June 3, 2024 14:51
@negvet
Copy link
Collaborator

negvet commented Jun 3, 2024

To discuss @negvet @goodsong81

openvino_xai.insertion -> openvino_xai.xai_branch_inserter

Rename to be more concise? xai_branch_inserter | xai_inserter | inserter | xai_ir_inserter

Assuming that one of this refactoring objective is to make code to be more concise, I would vote for the inserter.

@negvet
Copy link
Collaborator

negvet commented Jun 3, 2024

We need to have consistency between 3 options of calling Method, Task:

import openvino_xai as ovxai

ovxai.Task, ovxai.Method
from openvino_xai import Task, Method

Task, Method
from openvino_xai.common.parameters import Task, Method

Task, Method

I personally like both 2 and 3 option equally.

I would propose for the code under openvino_xai to be follow option 3.
But for the sample code under examples and docs, to follow option 1. So that the origin of the objects like Task and Method would be clear for the user and at the same time those object to be available at the top level (without going into the modules structure for import).

Copy link
Contributor

@goodsong81 goodsong81 left a comment

Choose a reason for hiding this comment

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

Thank you for the prompt renaming!
Looks good in general. I'd like to request a few minor changes.
Please review my comments.

p.s. Ah, please don't forget to update the license notices. :)

README.md Outdated Show resolved Hide resolved
examples/run_classification.py Outdated Show resolved Hide resolved
notebooks/xai_classification/xai_classification.ipynb Outdated Show resolved Hide resolved
openvino_xai/__init__.py Outdated Show resolved Hide resolved
openvino_xai/explainer/__init__.py Outdated Show resolved Hide resolved
openvino_xai/explainer/explainer.py Outdated Show resolved Hide resolved
openvino_xai/explainer/explanation_parameters.py Outdated Show resolved Hide resolved
openvino_xai/explainer/visualize.py Outdated Show resolved Hide resolved
goodsong81
goodsong81 previously approved these changes Jun 5, 2024
Copy link
Collaborator

@negvet negvet left a comment

Choose a reason for hiding this comment

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

Thank you for the update!

@negvet negvet enabled auto-merge June 6, 2024 07:17
@negvet negvet merged commit 8438854 into openvinotoolkit:develop Jun 6, 2024
4 checks passed
@goodsong81 goodsong81 added this to the 1.0.0 milestone Jun 11, 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.

3 participants