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.

Sign up for GitHub

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

Jump to bottom

image analysis abstract class #3

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

image analysis abstract class #3

wants to merge 6 commits into from

Conversation

nilix-ba
Copy link
Contributor

added the image analysis abstract class and open ai image analysis class. ALso, modified gemini image analysis assistant.

@floschne floschne self-requested a review January 31, 2025 14:09
floschne
floschne requested changes Jan 31, 2025
View reviewed changes
Copy link
Member

@floschne floschne left a comment

Choose a reason for hiding this comment

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

As discussed, please change the methods to generate prompts by returning only a string and, add it to the base class. Then you can use this in the Gemini and OpenAI subclasses.

floschne
floschne requested changes Feb 4, 2025
View reviewed changes
src/fundus_murag/assistant/openai_image_analysis_assistant.py

def _load_model(self, model_name: str, model_type: Literal["vqa", "ic"]):
def _load_model(self, model_name: str, model_type: Literal["vqa", "ic"]) -> ModelConfig:
Copy link
Member

Choose a reason for hiding this comment

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

Please update the method name to _build_model_config :-)

src/fundus_murag/assistant/openai_image_analysis_assistant.py

class ModelConfig(TypedDict):
Copy link
Member

Choose a reason for hiding this comment

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

Very nice idea to use a typed dict as a return type :-)

floschne
floschne requested changes Feb 4, 2025
View reviewed changes
Copy link
Member

@floschne floschne left a comment

Choose a reason for hiding this comment

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

With this commit you pushed a lot of changes which are either only for your local config or are not semantically coherent with the pull request (which is about the image analysis assistant only) -- please remove this commit (if you don't know how to do this, try to find out, there's a ton of Git Tutorials ;-) )

@nilix-ba nilix-ba closed this Feb 10, 2025
@nilix-ba nilix-ba reopened this Feb 10, 2025
@nilix-ba nilix-ba force-pushed the niloufar-dev branch 2 times, most recently from 98421f0 to b23fa35 Compare February 10, 2025 20:18
@nilix-ba
Copy link
Contributor Author

With this commit you pushed a lot of changes which are either only for your local config or are not semantically coherent with the pull request (which is about the image analysis assistant only) -- please remove this commit (if you don't know how to do this, try to find out, there's a ton of Git Tutorials ;-) )

It is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@floschne floschne floschne requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nilix-ba @floschne