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

Draw trivial tokens and indices #12

Merged
merged 8 commits into from
Feb 6, 2024

Conversation

emileferreira
Copy link
Contributor

Presently, drawing tokens or indices by themselves (for example, draw(tokens) or draw(indices, "hello")) results in an empty figure. This adds support for drawing trivial tokens and indices, as feedforward layers.

@gailweiss
Copy link
Contributor

this draws them, but mysteriously labels them with FF in the drawing (instead of X, as they are labeled when drawing computation flows with actual computations). is it possible to get them to show up the same way they would normally?

@emileferreira
Copy link
Contributor Author

emileferreira commented Jan 24, 2024

They're now labelled 'X' in the computational flow figures. It's not an elegant solution, but the existing code only considers feedforward layers and would otherwise need to be rewritten.

@gailweiss
Copy link
Contributor

if i understand correctly, you've treated them as their own parents, forcing them into the X row. it seems like a nice solution to me!

Can you add comments to the changes you've made, so it's clear why this special case (len(ffs)==1) exists now? i.e. specifically highlight around all your changes that len(ffs) == 1 implies we are drawing just a tokens/indices sop by itself, and (if it's confusing, eg switching ffs and their parents) explain what (and why) you are doing in this special case

@emileferreira
Copy link
Contributor Author

Thanks. I've added comments to explain the changes.

@gailweiss
Copy link
Contributor

gailweiss commented Feb 2, 2024

it's a nice trick, but i don't like the use of the 'constant' patch as it permanently marks that sequence as a constant, which later creates strange behaviour (e.g. if you do draw(indices+1); after having earlier done draw(indices);, you will now get a warning that it is drawing a constant). however, the parent-child swap for the base sequence draw was quite nice. i have made a slightly different set of changes, based on your own, which do not cause this issue. i will note them in comments on your code

@emileferreira
Copy link
Contributor Author

emileferreira commented Feb 5, 2024

I figured that it was safe to (ab)use the _constant property of UnfinishedSequence, as it's unused besides for that benign warning. I've resolved the warning by resetting the property after drawing.

@emileferreira
Copy link
Contributor Author

emileferreira commented Feb 6, 2024

The trivial case, in which the feedforward layers and their parents are to be swapped, is now identified by the length of the respective lists. This avoids using the _constant property and triggering the associated warning.

@gailweiss gailweiss merged commit 8fbbc67 into tech-srl:main Feb 6, 2024
3 checks passed
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