Conversation
Conflicts: regr/graph/dataNode.py regr/program/learningbaseprogram.py regr/program/model/pytorch.py regr/sensor/pytorch/sensors.py test_regr/examples/conll04/emr/data.py
Conflicts: regr/program/learningbaseprogram.py regr/program/model/pytorch.py
Conflicts: examples/emr/config.py examples/emr/emr/utils.py regr/program/loss.py regr/program/metric.py regr/utils.py
|
I merge the latest changes in conll04 example in Since the goal change to replace everything in this example with the new interface, we can merge it now and solve any remaining issue using the new interface. |
|
Should this PR be merged? Or it is WIP? |
| else: | ||
| return [(*output_raw, input_item) for input_item in input_raw] | ||
|
|
||
|
|
There was a problem hiding this comment.
It seems these are generic sensors not specifically for conll/EMR?
There was a problem hiding this comment.
Hi,
Yes, sensors in regr.sensor.torch are written in a generic manner. The only issue is their base class is regr.sensor.torch.Sensor. We later decided to use regr.sensor.pytorch.Sensor and subclasses instead of regr.sensor.torch.Sensor.
However, these sensors can be migrated easily to regr.sensor.pytorch.Sensor. I did some already, based on whatever I need in ACE event example.
There was a problem hiding this comment.
I see. So, this PR is not for merging then and needs cleaning up?
There was a problem hiding this comment.
This PR itself is good to merge because I just wanted to merge the latest changes in CONLL to develop.
I did not try to rewrite CONLL with the current interface in this PR. However, we can start another issue for this purpose.
There was a problem hiding this comment.
I think you better to clean this up and then merge. Those general sensors in EMR folder does not make sense. The develop branch becomes more chaotic given the lack of documentation.
examples/emr/emr/program/program.py
Outdated
|
|
||
|
|
||
| class LearningBasedProgram(): | ||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
why this class is under EMR? this class of LearningBasedProgram is only for EMR?
There was a problem hiding this comment.
Program class was first written for CONLL and I had made it generic and moved it to the base package. I did not realize there is this class remaining. I can remove it before merging.
There was a problem hiding this comment.
yes please clean this up, we should just merge the clean documented code, the repo becomes unmanageable it seems given the current approach to development.
There was a problem hiding this comment.
Sure. I am using this week to clean up and adding documentation, especially for Sensors and Programs.
I will clean up this class before merging this PR also.
examples/emr/emr/program/program.py
Outdated
| def populate_one(self, data_item, inference=True): | ||
| for key, value in data_item: | ||
| data_item[key] = [value] | ||
| return next(self.populate(data_item, inference)) |
There was a problem hiding this comment.
same comment, are these populate functions only for EMR? It would be helpful to add some documentation inside each class to see what is the goal, otherwise I am not sure if reviewing this code is possible. Thanks!
2 participants
No description provided.