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

Sentiment Heatmap as seperate page #42

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

Conversation

andrewscottcohen
Copy link

  • The sentiment map is not yet integrated into the pre-existing map (it will be really easy), however, the pre-existing map is currently not loading for me.
  • Go to /frontend/sentimentheatmap.html
  • Currently using dummy data source of CTA Bus Stops for heat map data, will need to switch over to Poems
  • Hit the 'Sentiment' Button in the top right corner

@nbriz
Copy link
Contributor

nbriz commented Dec 2, 2022

hey @andrewscottcohen , there are a few issues here preventing me from properly running + testing (&& thus reviewing) ur code, it might have to do with when u pulled from upstream && when u branched off of that to create ur fork. let's try to clean this up so i can pull + test on my end, here's what i suggest (as best as i can tell from here):

  • switch to ur main branch git checkout main && pull from upstream git pull upstream main (u may have merge conflicts, take care in resolving those, if u're not confident in that process let me know && we can schedule a time to zoom && do that together), once u've resolved those (which also means committing those changes), push to ur main branch git push origin main
  • then switch to ur new branch git checkout sentimentHeatmap && pull from the class main again git pull upstream main (u may need to resolve merge conflicts again), once u've resolved those push those updates to ur new branch git push origin sentimentHeatmap

once u do that let me know so i can pull on my end && see if this resolves the strange conflicts i'm getting

@andrewscottcohen
Copy link
Author

@nbriz just pushed the code after fixing the conflicts.

Copy link
Contributor

@nbriz nbriz left a comment

Choose a reason for hiding this comment

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

thnx @andrewscottcohen i was able to pull + test code now. great job creating a PR from a feature branch that conforms to our coding style. as for the code itself, few things before i can merge this:

first, there's 1 change u made which broke the original map which Allie + Ying worked on, u removed their accessToken (see comment below), we definitely want to revert that change && bring their token back to avoid breaking their code before i can merge this.

additionally, i'm not entirely sure how this is working at the moment, i remember u mentioned wanting to analyze the sentiment of the poems in our database && then displaying a layer over a map which paints it w/different colors based on the dominant sentiments in diff areas... but that doesn't seem to be going on right now.

i can see how u're using heatmap.js to create a map w/the heatmap layer on top of it, i can see that u're loading in that large CTA geojson file && then generating heatmaps based on that? (this is where i get a little lost in the code)

there's a file called sntmGeo.js, which seems to be aimed at generating that sentiment data, but u're not actually using this anywhere (also that sentiment library seems to be a backend library, see my comment below). there's also a file called data/testData.geojson which u don't seem to be using either

all this to say, this doesn't seem "finished", so we've got a couple options:

  1. either we discuss what/how needs to change in order to get this fully operationl
  2. we leave this here as a "proof on concept" to keep working on later, in which case we need to clean some of it up + document what's currently working + what needs to be worked on moving fwd.

what do u think?

@@ -1,5 +1,3 @@
mapboxgl.accessToken = 'pk.eyJ1IjoiYWpjaHUyOCIsImEiOiJja3o2M3MzMWswd200MnZwNGdieTNlaHRjIn0.BePZiTybP8rLoo6yQKon_w'
Copy link
Contributor

Choose a reason for hiding this comment

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

if u remove this accessToken from Allie + Ying's code then their maps don't work. we need to put this back into their file to avoid breaking it.

Copy link
Author

Choose a reason for hiding this comment

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

That was a mistake, not sure when that happened, but prob explains why their map wasn't loading for me.


// Sentiment Analysis package (installed from npm)
async function getSentiment (text) {
const Sentiment = require('sentiment')
Copy link
Contributor

Choose a reason for hiding this comment

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

sentiment is a node.js module meant to be used on the backend (this won't work on the frontend), the require() function is how we import modules (ie. libraries) on the backend, but that function doesn't exist on the frontend (ie. on the "client side", meaning it won't work in a browser, only on a server)

what u would want to do here is create an endpoint on our REST API which u can query for sentiment data by fetch()ing for it on the frontend

Copy link
Author

Choose a reason for hiding this comment

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

Got it, I'll work on that now.

@andrewscottcohen
Copy link
Author

@nbriz Right now the map is showing the density of bus stops in Chicago, not the actual sentiment. I did this to show a visual proof of concept while I waited for the rest api to be finalized so that I could compute sentiment across all poems. I heard your feedback on the backend sentiment function and I think I got that sorted out (could be wrong).

I'd be happy to hop on anytime to discuss the plan moving forward!

@nbriz
Copy link
Contributor

nbriz commented Dec 6, 2022

@andrewscottcohen just checking in on this, any idea when u think u'll be pushing these updates?

@andrewscottcohen
Copy link
Author

@nbriz I'll try to have it all in by tomorrow night. Do you have time tomorrow to hop on Zoom just to clarify what updates are needed?

@nbriz
Copy link
Contributor

nbriz commented Dec 7, 2022

@andrewscottcohen sure i'll email u some times

… proof of concept form page - sentiment function currently not calling from backend
@andrewscottcohen
Copy link
Author

@nbriz I merged the two maps and made the sentiment page. The sentiment function is not currently being read in the frontend.

@nbriz
Copy link
Contributor

nbriz commented Dec 8, 2022

@andrewscottcohen great!

the integration into the map.html looks good (don't forget to remove any old files we're not using anymore like sentimentHeatmap.html

&& the sentiment.html file && accompanying REST API end point are looking good so far let me know if u need any help w/that

@andrewscottcohen
Copy link
Author

@nbriz I'm having difficulty with the sentiment functions... I am getting a 404 error on the API request

@nbriz
Copy link
Contributor

nbriz commented Dec 12, 2022

@andrewscottcohen in order to use a library (like sentiment.js) u need to first download it using npm install [library] as they mention in their npm page, judging by ur package.json (which lists all the downloaded/installed "dependencies") i'm guessing u may have missed this step?

@andrewscottcohen
Copy link
Author

@nbriz That's all squared away and I made a few updates to the backend + frontend. I think it should be working now but it still is not. The input text still does not seem to run through the backend function.

@nbriz
Copy link
Contributor

nbriz commented Dec 13, 2022

thnx @andrewscottcohen, i'm going to mark the assignment as "complete" but i'm going to hold off on merging this since it's still a work in progress

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.

2 participants