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

Create API endpoint for proximity analysis #53

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

Create API endpoint for proximity analysis #53

wants to merge 15 commits into from

Conversation

yaroluchko
Copy link
Contributor

This pull request:

  • creates the API endpoint for the proximity analysis posting
  • creates an API endpoint for viewing all the proximity analyses
  • adds the urls for the two API endpoints above
  • adds a manual test in the proximity_test.py file

Watermelanie and others added 11 commits July 21, 2021 17:09
@Watermelanie @yaroluchko
created an API endpoint for posting proximity analysis
f3114ec 
Co-Authored-By: yaroluchko <[email protected]>
@joshfeli
Resolved __hash__ error
4d88605
@joshfeli
Merge branch 'main' into create_API_endpoint_for_proximityAnalysis
efdee64
@yaroluchko
Merge branch 'main' into create_API_endpoint_for_proximityAnalysis
c48ca53
@yaroluchko
Merge branch 'create_API_endpoint_for_proximityAnalysis' of https://g…
6a5b6f9 
…ithub.com/dhmit/gender_analysis_web into create_API_endpoint_for_proximityAnalysis
@yaroluchko
Merge branch 'main' into create_API_endpoint_for_proximityAnalysis
adf0f5d
@yaroluchko
Proximity Analysis API endpoint
0f8b7aa 
This creates the Proximity Analysis API endpoint and a way to see all the analyses
@yaroluchko
A fix to make the key a string instead of Gender
4da0311
@yaroluchko
Create proximity_test.py
e4ee358 
A manual test for proximity posting
@yaroluchko
Update views.py
191daa6 
Fix unncessary result calculation if already done before
@yaroluchko
Update views.py
30ef395
@joshfeli joshfeli requested a review from MBJean July 22, 2021 23:37
@joshfeli
Copy link
Contributor

Just to note: The proximity_frontend branch has this copied and pasted to test the endpoint with the button in place, and all data passes through successfully! We were able to at least get the console to print out the results object, but this will be replaced with a display function.

MBJean
MBJean reviewed Jul 23, 2021
View reviewed changes
Copy link
Contributor

@MBJean MBJean left a comment

Choose a reason for hiding this comment

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

This generally looks great! Thanks for your updates. I've left a few comments below. I'd be interested in seeing a more comprehensive test, if possible.

backend/app/views.py
'word_window': word_window,
'results': results,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Ultimately, we may want to change this up so that you call a ProximityAnalysisManager or similar (like we do with the DocumentManager) when we create this new ProximityAnalysis, and have the ProximityAnalysisManager call the run_analysis function. That is: since a ProximityAnalysis necessarily relies on the run_analysis function, we could use the ProximityAnalysis itself (through a Manager) to ensure that run_analysis is run when a new instance is created. That would more tightly couple associated functionality. That said, since this is working and is well tested, we can leave it be for now and come back to it later.

backend/app/analysis/proximity_test.py Outdated
factory = APIRequestFactory()
request = factory.post('api/all_proximity', {'word_window': '2', 'corpus_id': c1.id})
add_proximity_analysis(request)

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this test used simply to manually ensure that the full ProximityAnalysis creation occurred as expected? Would it be possible to add something to our larger test suite so we can check that the model instance is saved as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, this was just a manual test. I will try and add a more comprehensive test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just added a test to the tests.py file that makes sure the post request is successful

yaroluchko and others added 4 commits July 23, 2021 15:51
@yaroluchko
Test Case for proximity analysis object creation
e2c5448
@yaroluchko
Delete proximity_test.py
2130b24
@joshfeli
Altered import statement
41382a6 
`frequency.py` also has a `run_analysis` function; this change will prevent aliasing errors should the other team import the `frequency` counterpart into `views.py`.
@joshfeli
Squashed commit of the following:
bfa0060 
commit 0fdfcc6
Merge: 1276da0 b10cfa9
Author: Peihua Huang <[email protected]>
Date:   Fri Jul 23 15:48:02 2021 -0400

    Merge pull request #25 from dhmit/transfer_frequency_module_PR1

    Transfer frequency module pr1

commit 1276da0
Merge: 657b20f b5a9482
Author: YifanWang0 <[email protected]>
Date:   Fri Jul 23 15:38:12 2021 -0400

    Merge pull request #54 from dhmit/transfer_frequency_model

    Added FrequencyAnalysis model

commit b5a9482
Author: Yifan Wang <[email protected]>
Date:   Fri Jul 23 14:08:54 2021 -0400

    added FrequencyAnalysis model

commit b10cfa9
Author: Peihua Huang <[email protected]>
Date:   Fri Jul 23 13:41:17 2021 -0400

    update run_analysis to use primary keys instead of labels and update result dictionary to key by gender object

commit 1196fc1
Author: Peihua Huang <[email protected]>
Date:   Fri Jul 23 13:36:54 2021 -0400

    Revert "update frequency result to key by gender primary key instead of label"

    This reverts commit 151a4bc.

commit 151a4bc
Author: Peihua Huang <[email protected]>
Date:   Fri Jul 23 13:34:11 2021 -0400

    update frequency result to key by gender primary key instead of label

commit 657b20f
Merge: 6619e68 3c6e5bf
Author: Joshua Feliciano <[email protected]>
Date:   Fri Jul 23 11:42:15 2021 -0400

    Merge pull request #52 from dhmit/remove_proximity_breakpoint

    Removed debugging breakpoint

commit 3c6e5bf
Author: Joshua Feliciano <[email protected]>
Date:   Thu Jul 22 17:50:15 2021 -0400

    Removed debugging breakpoint

commit 0684a02
Author: Peihua Huang <[email protected]>
Date:   Thu Jul 22 16:43:50 2021 -0400

    got frequency analysis working with a corpus

commit 2003ce6
Author: Peihua Huang <[email protected]>
Date:   Thu Jul 22 16:28:27 2021 -0400

    convert run_analysis to run_single_analysis, such that the function only takes in one document and update docstrings and added test

commit 0f817ba
Merge: dc21391 6619e68
Author: Peihua Huang <[email protected]>
Date:   Thu Jul 22 14:54:15 2021 -0400

    Merge branch 'main' into transfer_frequency_module_PR1

commit dc21391
Author: Yifan Wang <[email protected]>
Date:   Wed Jul 7 16:44:55 2021 -0400

    fixed small mistakes according to pr comments

commit a852cd3
Author: Yifan Wang <[email protected]>
Date:   Mon Jun 28 16:42:28 2021 -0400

    added docstrings

commit 32731f4
Author: Yifan Wang <[email protected]>
Date:   Mon Jun 28 16:01:57 2021 -0400

    fixed minor bug with _run_analysis function

commit 233e2e6
Author: Yifan Wang <[email protected]>
Date:   Fri Jun 25 16:52:31 2021 -0400

    moved over run analysis but still need to fix bug with helper function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MBJean MBJean MBJean left review comments

Assignees

@farooqashar farooqashar

@joshfeli joshfeli

@Watermelanie Watermelanie

@irinazoccolini irinazoccolini

@yaroluchko yaroluchko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yaroluchko @joshfeli @MBJean @farooqashar @Watermelanie @irinazoccolini