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

Modification of the definition of networks #18

Open
wants to merge 63 commits into
base: doc_refactor
Choose a base branch
from

Conversation

SeguinBe
Copy link
Collaborator

So now the network definition is based on two bases classes, an Encoder and a Decoder.

This simplifies quite a bit of things, and allows more flexibility. Also, it will allows anyone to plug-in its own network definition without having to clone dh_segment.

@SeguinBe SeguinBe requested a review from solivr October 12, 2018 18:32
@SeguinBe SeguinBe self-assigned this Oct 12, 2018
if self.blocks >= 5:
net = layers.repeat(net, 3, layers.conv2d, 512, [3, 3], scope='conv5')
intermediate_levels.append(net)
net = layers.max_pool2d(net, [2, 2], scope='pool5')
Copy link
Member

Choose a reason for hiding this comment

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

The output of the last max_pool2d is not returned, shouldn't it be ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mmhh... good catch. In a way, it depends what we want to do with the model definition, it can make sense to keep the feature maps with the best resolution but the lower receptive field. But I think if we want to be consistent with what we had, outputs should have the pooled versions, thoughts?

@@ -13,7 +13,7 @@ class PredictionType:
MULTILABEL = 'MULTILABEL'

@classmethod
def parse(cls, prediction_type):
def parse(cls, prediction_type) -> 'PredictionType':
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unrelated to this diff, but I wonder if we use Enum we don't get a parse method for free?

@solivr solivr closed this Nov 5, 2018
@SeguinBe
Copy link
Collaborator Author

SeguinBe commented Nov 5, 2018 via email

@solivr
Copy link
Member

solivr commented Nov 5, 2018

oops sorry I closed doc_refactor branch, but forgot that this one is related to it...

@solivr solivr reopened this Nov 5, 2018
@wrznr
Copy link

wrznr commented Apr 17, 2019

Is it recommended to use this branch instead of master? It seems to have a lot of fixes and extensions...

@rkeiii
Copy link

rkeiii commented Apr 27, 2019

I just wanted to note I was unable to run demo.py on the master branch or the latest release tag. It would fail with a ValueError. That being said, demo.py ran just fine on this encoder_revamp branch. It's somewhat confusing to a newcomer like me that this isn't noted anywhere.

@wrznr
Copy link

wrznr commented May 2, 2019

A lot of the changes suggested here are actually part of the master branch. 😕

@solivr
Copy link
Member

solivr commented May 15, 2019

@wrznr The official branch is the master branch. This one should work as well, but I'm not sure it has been tested and we need to make sure it behaves like master (or better) before merging it. It will hopefully be merged some day but I am not working on this at the moment, and AFAIK no one else is. Sorry for that.

@SeguinBe
Copy link
Collaborator Author

To pursue on @solivr comment, we do not have anybody continuing the development at the moment ( in fact I have not been part of the lab anymore for some time already).

A lot of the commits are actually merged from master though so it might appear that there is much more modifications than there are actually.

@SeguinBe
Copy link
Collaborator Author

SeguinBe commented May 22, 2019

We did a bit of merging today, things left to correct.

  • check the performance between this branch and the old one to verify it is still working properly
  • update the setup.py packages for the version numbers to be more flexible
  • update the installation documentation so that pip install after tensorflow installation is the preferred method (reminder: tensorflow can be installed with anaconda or pip, then pip install dh-segment will not install tensorflow)
  • update the general documentation
  • update the demo, ideally a zip file containing training data and a config file.

@solivr
Copy link
Member

solivr commented Aug 14, 2019

  • I have trained two models for each branch on two different tasks (baseline and page extraction). The optimal post-processing parameters seem to be different for the two branches, but models can achieve similar results.
  • The package versions are now more flexible in setup.py (in master).
  • Installation is now updated (pip install followed by conda's tensorflow installation) (in master).
  • Two examples of data generation, training and inference are now available in exps folder (master).

This branch seems ready to be merged with master.

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.

6 participants