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

initial implementation #2

Closed
wants to merge 13 commits into from
Closed

initial implementation #2

wants to merge 13 commits into from

Conversation

selenasong
Copy link
Contributor

fixes #1

Copy link
Member

@keighrim keighrim left a comment

Choose a reason for hiding this comment

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

Most importantly, consider upgrade to clams-python==1.2.4. To do that, I suggest delete all files from clams develop command (keep a copy of changed files of course) and re-run clams develop from 1.2.4 to start over. There are major changes in the starter code that's not so trivial to apply to starter code from <1.2.4, and probably easier just to start over.

metadata.py Outdated
url="https://github.com/clamsproject/app-tfidf-keyword-detector", # a website where the source code and full documentation of the app is hosted
# (if you are on the CLAMS team, this MUST be "https://github.com/clamsproject/app-tfidf-keyword-detector"
# (see ``.github/README.md`` file in this directory for the reason)
analyzer_version='version_0.0.1', # use this IF THIS APP IS A WRAPPER of an existing computational analysis algorithm
Copy link
Member

Choose a reason for hiding this comment

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

this app is not a "wrapper" of an off-the-shelf app/model, but something you develop from scratch, so no need to put anything here.

metadata.py Outdated
# first set up some basic information
metadata = AppMetadata(
name="Tfidf Keyword Detector",
description="", # briefly describe what the purpose and features of the app
Copy link
Member

Choose a reason for hiding this comment

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

say something about the app, maybe including some "documentation" of the training data and model status, etc.

requirements.txt Outdated
@@ -0,0 +1,2 @@
# Make sure clams-python version is explicitly specified, at least the lower bound
clams-python==1.2.2
Copy link
Member

Choose a reason for hiding this comment

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

  1. Please consider update to clams-python 1.2.4, That will come with cli.py by default when you do clams develop command
  2. Please provide the list of dependencies. Our convention: strict pin the clams-python and pin lower bound for other libraries

app.py Outdated
# create the document to store the keywords
keywords_doc = new_view.new_textdocument(text=keywords)

a = new_view.new_annotation(Uri.TEXT)
Copy link
Member

Choose a reason for hiding this comment

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

This is just wrong use of annotation type. Please read https://mmif.clams.ai/1.0.4/ and all the example MMIFs linked in the spec documentation. LAPPS-based types must be come from LAPPS type hierarchy (https://vocab.lappsgrid.org/) not from LAPPS sub-namespace.

app.py Outdated
parsed_args = parser.parse_args()

# create the app instance
app = TfidfKeywordDetector(parsed_args.idf_file, parsed_args.feature_dict_file, parsed_args.topn)
Copy link
Member

Choose a reason for hiding this comment

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

when migrating to 1.2.4, this should be written in a quite different way. Please refer to the in-line command in the starter code.

@keighrim
Copy link
Member

superseded by #3

@keighrim keighrim closed this Jul 19, 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.

input/output specification
2 participants